Request for reviews (XL): 6829193: JSR 292 needs to support SPARC
Christian.Thalinger at Sun.COM
Wed Apr 21 07:14:40 PDT 2010
On Tue, 2010-04-20 at 16:05 -0700, John Rose wrote:
> 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.
Well, logStackElementSize already exists, so I thought using it is a
good idea. Anyway, I change it back.
> --- 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
> --- 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. :-)
Hehe. I fix the comment.
> verify_argslot, etc.: Have you tested this code with the debug build?
> And also with -XX:+VerifyMethodHandles?
debug build, yes. VerifyMethodHandles, no. Will do so right now...
> STACK_BIAS: First, it is zero on ILP32, so it should be protected by
> 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.
I tried different implementations but I think something like a
regcon_add_align_sp would be best. I will post a webrev with this one.
> 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
> + // - O0: receiver method handle
> Should be G3_method_handle, right?
More information about the hotspot-compiler-dev