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

[Christian Thalinger]

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'.

Both done.

> 
> --- 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.  :-)

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...
looks good.

> 
> 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.

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.

Done.

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

Right.  Fixed.

-- Christian