Request for reviews (XL): 6829193: JSR 292 needs to support SPARC

John Rose john.r.rose at oracle.com
Tue Apr 20 16:05:26 PDT 2010


Review comments:

In general, nice comments added in various places explaining what's going on.

--- assembler_sparc
In MacroAssembler::argument_offset I prefer the formulation with exact_log2, rather than introducing another constant (logStackElementSize).  The slowness of exact_log2 doesn't matter, and the precise correspondence with its operand does matter.

--- templateInterpreter_sparc
(Note how much buggy garbage code goes away from generate_return_entry_for if callers pop arguments on call rather than on return.)

--- templateTable_sparc
+ // XXX can I use G1 here
Yes; you can just delete the query.

'G5_method': suggest using a locally named temp called 'G5_callsite'.

--- methodHandles_x86, methodHandles

The light changes to x86 code cry out for a refactor of sparc-specific code into methodHandles_sparc.hpp.  We have this in the mlvm already, as part of meth-conv-6939861.patch.  I guess we can wait for that change set to fix this, but it would have been nice to introduce the arch-specific header file with the second arch, rather than later.

--- interp_masm_sparc, interpreter_sparc, stubGenerator_sparc, assembler_x86, methodHandles_x86
OK.

--- methodHandles_sparc
The biggie...

The comments and variable names read well.

It parallels the x86 code nicely.  This is important, because the x86 code is changing.

+   // emit WrongMethodType path first, to enable jccb back-branch from main path
Now we just have to notify the SPARC hardware designers that we are ready for the jccb instruction.  :-)

verify_argslot, etc.:  Have you tested this code with the debug build?  And also with -XX:+VerifyMethodHandles?

STACK_BIAS: First, it is zero on ILP32, so it should be protected by LP64_ONLY.
Second, you can't do this on LP64:
  add(SP, STACK_BIAS, SP);
...because if an interrupt comes in on that thread, the OS won't know how to walk the stack and find register windows, etc.

Instead, correct 'offset' (maybe into a temp register), and then just add to SP.  I think it's worth defining a MacroAssembler macro for correcting a RegisterorConstant value.  Use regcon_sll_ptr_by_con as a model, and define regcon_align_ptr_by_con.

In remove_arg_slots, you should increment SP.  Use the the alignment macro, of course.

In generate_method_handle_stub, I think the following comment is wrong:
+   // - O0: receiver method handle
Should be G3_method_handle, right?

...To be continued...

-- John



More information about the hotspot-compiler-dev mailing list