Skip to content

Created baseline compiler for 32-bit ARM #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Created baseline compiler for 32-bit ARM #11

wants to merge 2 commits into from

Conversation

dmitry-brizhinev
Copy link
Contributor

I submit for your consideration the ARM baseline compiler.

It's not quite ready yet. It passes all of the pre-commit tests, but not all of the jni, dacapo and gctest ones.
However, it is in its final form and I think now would be a good time for some code review so I can fix any issues.

@erik-brangs
Copy link
Member

Thanks. I've begun looking at the code and will give you feedback later this week.

Could you post a message to the core mailing list to let us know about your plans for integration? I'd like to know if you plan to do this via pull request(s) or patches. I also want to know if there are any plans to set up regression testing for ARM.

@erik-brangs
Copy link
Member

The implementation looks mostly good AFAICT.

General points

  • it would be nice if the JNI implementation could use the entry and exit methods that IA32 uses. In fact, the PPC implementation also ought to use them.
  • there are a few places (e.g. CompilerDNA and OptCompiledMethod) where you're using RuntimeExceptions for unimplemented code. You could use assertions in those. If you do want to use Throwables, use UnimplementedError.
  • the documentation needs work. In particular, some methods ought to have JavaDoc documentation instead of normal inline comments. Many JavaDoc comments are missing formatting elements. This makes the JavaDoc less readable. You can simply compile the JavaDoc documentation for the new classes and check if it looks right. The existing JavaDoc documentation has enough examples that use formatting elements.
  • there shouldn't be any errors when running "ant apidoc-for-all-architectures".
  • the documentation for some files is just plain wrong because it wasn't updated yet (see below)
  • there are a few files with 2 statements per line. This should be avoided because it generally decreases readability. Multiple statements per line are fine if they enhance readability (e.g. as in case statements).

Features to think about

  • all Jikes RVM compilers (including the current ARM baseline compiler implementation) don't treat the this pointer for synchronized methods correctly. See https://xtenlang.atlassian.net/browse/RVM-1017 for a discussion. You need to decide if you want to fix that for the ARM implementation or not.
  • There's code for a Java implementation for the ldiv and lrem bytecodes on the issue tracker ( https://xtenlang.atlassian.net/browse/RVM-1114 ). I haven't merged it yet because it would require testing on PPC32 which I don't have access to. If you think it's useful for the ARM port, we could try to adjust the patch so that we can merge it for IA32 and ARM.
  • the code doesn't have any checks that use ExtremeAssertions. ExtremeAssertions is intended for expensive runtime checks that are useful to detect bugs. For example, we have checks that check stack alignment (IA32 JNI compiler) and non-movability of objects (IA32 barriers). If there are any useful but expensive checks, you could add them, guarded by VM.ExtremeAssertions.

Comments for files

  • StackFrameLayout: the returns should have their own line.
  • RegisterConstants: I don't understand the purpose of the DX registers. If they aren't valid, why have them? If you want to keep them, please add appropriate documentation. If you keep them, you could also introduce a method to factor out the (result & 1) == 0 check.
  • StackframeLayoutConstants: the picture would be clearer if you actually wrote (baseline compiler) instead of (++) or mentioned the compiler type before the picture. Some of the lines in the picture aren't properly formatted.
  • UnboxedType: rather than adding an assertion, refactor to something like memoryBytes = ArchConstants.getInstructionWidth()
  • ArchBaselineCompiledMethod: You'll save on annotations if you mark the class as @Uninterruptible and saveCompilerData as @Interruptible
  • CodeArray: The backing data for PPC and ARM code arrays is the same. This could be refactored to just refer to fixed length instructions (PPC & ARM) in int[] and variable-length instructions (IA32) in byte[].
  • OptCompiledMethod: You could introduce a new Configuration property for the requirement to synchronize instruction and/or data caches.
  • Time: cycles() should be rewritten to return (!VM.BuildForARM) ? Magic.getTimeBase() : nanoTime();
  • JNIFunctions: Change the comments to "NOTE: This implementation is ONLY used for PPC" instead. PPC is really the architecture that ought to be fixed.
  • BaselineCompilerImpl:
    • a link to the official documentation of calling conventions would be nice if the link is reasonably unlikely to change
    • some methods later in the source code should have JavaDoc comments instead of single-line comments (//)
    • there's 2 variants of "use nonvolatile registers". there should be at most one and it should be documented.
    • emit_iconst: the comment is confusing. If the method has limits on what it can handle, it should have an assertion.
    • genPrologue: according to the comments in the function, R12 is clobbered in the stack overflow check. Afterwards, it is saved for dynamic bridge frames. This doesn't make sense to me.
    • MagicNames.addressArraySet: yes, I don't think there should be any barriers for unboxed types. This is probably wrong on PPC.
    • MagicNames.isync / MagicNames.sync: the correct way to fix this is to change the generic code where necessary. For example, some uses are probably intended as a memory barrier. There are dedicated magic methods for memory barriers now. It's also possible to rename the methods to make it clear that they're architecture-independent (e.g. synchronizeInstructions instead of isync).
  • Assembler
    • all generic fits and mask methods should be replaced with methods from Bits and/or moved to that class.
    • the bit patterns are hard to read because there isn't any kind of ordering that is immediately apparent. You could name those after the instructions (e.g. as in the PPC assembler) or add a few comments on how the patterns are ordered.
    • emitNOP: this could just call emitNOP(ALWAYS). A helper method that emits several NOPs might be useful if you need code patching (e.g. for an opt compiler implementation).

Wrong and/or misleading (JavaDoc) comments

The documentation for the following files needs to be fixed:

  • sysThread_arm.c
  • ArchBaselineCompiledMethod
  • Barriers
  • JNICompiler

@erik-brangs
Copy link
Member

Is the ARM port still being developed?

@steveblackburn
Copy link
Contributor

steveblackburn commented Aug 24, 2017 via email

@dmitry-brizhinev
Copy link
Contributor Author

dmitry-brizhinev commented Aug 24, 2017 via email

@erik-brangs
Copy link
Member

I have some patches for a rebased version of this pull request. If anyone is still interested and has hardware to test, I could send them to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmitry-brizhinev @erik-brangs @steveblackburn