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