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

[Christian Thalinger]

On Jun 2, 2011, at 12:21 PM, Tom Rodriguez wrote:
>> Does this indicate a bug on amd64?
>> -    int  slot_size     = (arg_size > wordSize) ? arg_size : wordSize;
>> +    int  slot_size     = is_subword_type(type) ? type2aelembytes(T_INT) : arg_size;  // store int sub-words as int
>> 
>> Or is it just a big-endian thing?  If it's an amd64 bug let's fix it.
> 
> I didn't write that so I'm not sure but it looks like an endianness issue.

Yes, that's an endian issue.

> 
>> 
>> This line makes me uneasy:
>> +    __ store_sized_value(O0, return_slot, type_size);
>> 
>> The return_slot is the address of a "hole" in the stacked argument list.  It's going to get passed to the final target of the RF.
>> So whatever gets stored into it will be all the information in the stack slot for that argument.
>> I think what you wrote works only because there was a "42" in the hole already.  If there is garbage, and you store a byte on top, you get a partly garbage value.
> 
> Again, I don't know why it was written that way.  It seems like it should be using type2alembytes(T_INT).  I've adjusted it.

Not sure about that one, I need to look at it again.  But type2alembytes(T_INT) sounds reasonable.

>> This seems like you don't need it:
>> +        // reload from rdx_argslot_limit since rax_argslot is now decremented
>> +        __ ld_ptr(Address(O2_argslot_limit, -Interpreter::stackElementSize), O1_array);
>> 
>> The reload is on the x86 side because of the scarcity of registers.
> 
> The previous line appear to use O1 as a temp:
> 
>        insert_arg_slots(_masm, O3_stack_move,
>                         O0_argslot, O4_scratch, G5_scratch, O1_scratch);
>        // reload from rdx_argslot_limit since rax_argslot is now decremented                                                                        
>        __ ld_ptr(Address(O2_argslot_limit, -Interpreter::stackElementSize), O1_array);

Right.  I tried to get away without the reload but it didn't work out.

Thanks Tom for fixing the remaining problems and pushing it.  Thanks John and Vladimir for the reviews.

-- Christian