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

[Tom Rodriguez]

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?

> 
> methodHandles_sparc.cpp:
> 
> Can you do the decrement of slot_num as separate expressions?:

-= is the same as predecrement not postdecrement, right?

> 
> +   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];

> 
> + // Emit code to verify that RBP is pointing at a valid ricochet frame.
>                              ^ no RBP on sparc

Fixed.

> 
> RicochetFrame::verify_clean() has commented out code.

I've removed it.

> 
> Should next code use brx() instruction?:
> 
> +   __ ld_ptr(L4_saved_args_base, __ argument_offset(O5_temp), O7_temp);
> +   __ cmp(O7_temp, (int32_t) RETURN_VALUE_PLACEHOLDER);
> +   __ br(Assembler::equal, false, Assembler::pt, L_ok_4);

Probably.

> 
> 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);
}

> 
> MethodHandles::verify_stack_move() has commented code. Should UNREASONABLE_STACK_MOVE depends on LP64?

I'm going to rework this.

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

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.