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

[Tom Rodriguez]

Resending to list because it bounced.

This change:

+  void get_cache_entry_pointer_at_bcp(Register cache, Register tmp, int bcp_offset, bool giant_index = false);

seems spurious.

I think the return_entry_for changes aren't really in the spirit of the design of the template interpreter.  If invokedynamic has it's own strategy for lookup in the constant pool then it probably needs it's own return entry point.  The invokedynamic case would select a new table in prepare_invoke that was generated by a call to generate_return_entry_for with a new flag indicating that the bytecode being handled is invokedynamic so it could use the giant cp cache logic.  What you've got is ok but it's not really the way the template interpreter works.

regcon_sll_ptr_by_con sure is convenient but its implementation is totally surprising and the name is long and unclear particularly since it has the same signature as regcon_sll_ptr but does something different.  It desperately needs a comment in the hpp explaining what it does.  I don't think dst should ever be optional, otherwise why it at all.  This use:

+      __ add(Gargs, __ regcon_sll_ptr_by_con(G5_stack_move, LogBytesPerWord, G5_stack_move), Gargs);

should just be sll_ptr.  Actually given all the magic that the existing regcon_sll_ptr does, couldn't you shoehorn the new behaviour into it?  The by_con behaviour of returning the new regcon instead of the value return parameter seems better anyway.

The switch in load_sized_value is too clever for my taste.  How about just a switch on size_in_bytes with a test on is_signed where it's needed?

I'll leave the big blobs of method handle code to John.

tom

On Apr 19, 2010, at 5:50 AM, Christian Thalinger wrote:

> On Fri, 2010-04-09 at 09:34 -0700, Vladimir Kozlov wrote:
>> On 4/9/10 8:13 AM, Christian Thalinger wrote:
>>>> 
>>>> MethodHandles::generate_method_handle_interpreter_entry():
>>>> 
>>>> !     Register tem = G5_method;
>>>> !     for (jint* pchase = methodOopDesc::method_type_offsets_chain(); (*pchase) != -1; pchase++) {
>>>> !       __ ld_ptr(tem, *pchase, G5_method_type);
>>>> !       tem = G5_method_type;       // yes, it's the same register...
>>>>             ^ it is loop invariant, why you put it inside?
>>>> !     }
>>> 
>>> I'm not sure I understand.  G5_method_type is loaded in the ld-ptr
>>> instruction from Address(tem, *pchase).
>> 
>> tem = G5_method_type is register renaming - it is not moving value,
>> so you don't need to do renaming inside loop.
> 
> Ahh, got it.  That is because of the porting from x86.  I removed the
> code from the loop and added an assert.
> 
>>>> 
>>>> Also MacroAssembler::load_method_handle_vmslots() has next code which
>>>> looks similar to above but has the check "!= 0"
>>>> 
>>>> !   if (java_dyn_MethodHandle::vmslots_offset_in_bytes() != 0) {
>>>> !     // hoist vmslots into every mh to avoid dependent load chain
>>>> !     ld(    Address(mh_reg,    delayed_value(java_dyn_MethodHandle::vmslots_offset_in_bytes, temp_reg)),   vmslots_reg);
>>>> !   } else {
>>>> !     Register temp2_reg = vmslots_reg;
>>>> !     ld_ptr(Address(mh_reg,    delayed_value(java_dyn_MethodHandle::type_offset_in_bytes, temp_reg)),      temp2_reg);
>>>> !     ld_ptr(Address(temp2_reg, delayed_value(java_dyn_MethodType::form_offset_in_bytes, temp_reg)),        temp2_reg);
>>>> !     ld(    Address(temp2_reg, delayed_value(java_dyn_MethodTypeForm::vmslots_offset_in_bytes, temp_reg)), vmslots_reg);
>>>> !   }
>>> 
>>> You want me to reuse that code?
>> 
>> No. I was confused that in load_method_handle_vmslots() there is separate code when vmslots_offset_in_bytes() != 0 and 
>> generate_method_handle_interpreter_entry() does not have such code.
> 
> Hmm.  I don't know the details here (John should) but maybe that's
> because we dig out the right MethodType and then retrieve the vmslots value from
> that MethodType and the short-cut is not valid at that point.  But
> that's just a guess.
> 
> Here is the updated webrev:
> 
> http://cr.openjdk.java.net/~twisti/6829193/webrev.03/
> 
> -- Christian
>