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

[Vladimir Kozlov]

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.

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

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