review for 7045514: SPARC assembly code for JSR 292 ricochet frames

[Tom Rodriguez]

On Jun 2, 2011, at 9:20 AM, Vladimir Kozlov wrote:

> On 6/1/11 9:10 PM, Tom Rodriguez wrote:
>> 
>> On Jun 1, 2011, at 6:15 PM, Vladimir Kozlov wrote:
>> 
>>> Christian is hero to write all this :)
>> 
>> Yep.
>> 
>>> 
>>> Did you test with -XX:-EnableInvokeDynamic (off)?
>> 
>> No.  Are you asking if we can run normal programs with -EnableInvokeDynamic?
> 
> Yes, this flag is on by default so I want to know if we can run with it off.

It seems to work with it off.

> 
>> 
>>> 
>>> methodHandles_sparc.cpp:
>>> 
>>> Can you do the decrement of slot_num as separate expressions?:
>> 
>> -= is the same as predecrement not postdecrement, right?
> 
> I am not sure, this is why is why I asked to separate it.
> 
>> 
>>> 
>>> +   intptr_t* loc =&base[slot_num -= 1];
>> 
>>   int slot_num = slot_count - 1;
>>   intptr_t* loc =&base[slot_num];
>> 
>>> 
>>> +     loc =&base[slot_num -= type2size[ptype]];
>> 
>>     slot_num -= type2size[ptype];
>>     loc =&base[slot_num];
>> 
> 
> Good.
> 
>>> 
>>> In load_conversion_vminfo() could you just use the same assert (CONV_VMINFO_SHIFT == 0) as next method and simplify address expression.
>> 
>> This?
>> 
>> void MethodHandles::load_conversion_vminfo(MacroAssembler* _masm, Address conversion_field_addr, Register reg) {
>>   assert(CONV_VMINFO_SHIFT == 0, "preshifted");
>>   assert(CONV_VMINFO_MASK == right_n_bits(BitsPerBte), "else change type of following load");
>>   __ ldub(conversion_field_addr.plus_disp(BytesPerInt - 1), reg);
>> }
> 
> Good.
> 
>> 
>>> 
>>> Could you move following verification code under first condition?:
>>> 
>>> +       if (collect_count_constant>= 0) {
>>> +         collect_count = collect_count_constant;
>>> +       } else {
>>> +         __ load_method_handle_vmslots(O1_collect_count, G3_method_handle, O2_scratch);
>>> +         collect_count = O1_collect_count;
>>> +       }
>>> + #ifdef ASSERT
>>> +       if (VerifyMethodHandles&&  collect_count_constant>= 0) {
>>> +         BLOCK_COMMENT("verify collect_count_constant {");
>>> +         __ load_method_handle_vmslots(O3_scratch, G3_method_handle, O2_scratch);
>>> 
>>> The same for dest_slot_constant verification.
>> 
>> I can join them but I think I might swap the conditions.
> 
> Swap is fine.

Fixed.

tom

> 
> Thanks,
> Vladimir
> 
>> 
>> tom
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/7045514
>>>> 2389 lines changed: 1833 ins; 363 del; 193 mod; 46455 unchg
>>>> 7045514: SPARC assembly code for JSR 292 ricochet frames
>>>> Reviewed-by:
>>>> This is the complete support for Ricochet frames on sparc.  Christian
>>>> did all the work and testing and I just did some final testing and bug
>>>> fixing.
>>>> A potential issue with checkcasts reusing locals in
>>>> methodHandleWalk.cpp was fixed.  Comments weren't being transferred
>>>> onto the MethodHandlesAdapterBlob.  A derived oop issue was found
>>>> where an assert was complaining that an offset was too large but
>>>> there's no real restriction on the offset of derived oops so I
>>>> disabled the assert.  ResourceMarks were added in verification logic.
>>>> A verify_vmargslot call was verifying against the wrong signature
>>>> resulting in occasional incorrect exceptions.  I updated the
>>>> MacroAssembler::debug assertion messages to include the passed in
>>>> message.  Many of blob declarations were moved into shared code so
>>>> that we don't have to replicate code.  Some x86 method handles code
>>>> was changed to make signatures match.
>>>> Currently it passes all the jdk regression tests on sparc but I can't
>>>> run any of the others because of version mismatches between the JDK
>>>> and the tests.  Earlier versions ran those as well they had
>>>> previously.  I also ran the jruby tests and those seemed clean as well.
>>