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

[Tom Rodriguez]

On Jun 1, 2011, at 7:58 PM, John Rose wrote:

> On Jun 1, 2011, at 2:38 PM, Tom Rodriguez wrote:
> 
>> 7045514: SPARC assembly code for JSR 292 ricochet frames
> 
> --- methodHandles_sparc.hpp
> 
> Looks good.
> 
> --- methodHandles_sparc.cpp
>  // Gargs points to the first word so adjust by BytesPerWord
> Heavy sigh.  IMO, that off-by-one design is a plague on our source base.
> (Also, later:  __ add(FP, STACK_BIAS - (1 * Interpreter::stackElementSize), Gargs))

Yes, it's ugly.

> 
> This expression assumes a tricky relation (including an exp2(1+log2(n))) between the two constants:
>  (offset + 1*BytesPerWord) & ~TwoWordAlignmentMask
> 
> I would prefer simply:
>  round_to(offset, TwoWordAlignmentMask+1 /*or whatever*/)

Fixed.

> 
> There's also a __ round_to(reg, mod) to use on the other branch.  Could be a __ round_down also.

I'd prefer not to futz with that now.  It's too easy to get wrong.

> 
> (Nice factorization of the tricky bits in adjust_SP_and_Gargs_down_by_slots etc.)
> 
> Is it worth testing that the incoming value to adjust_... is always >= 0?

Added.

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

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

> 
> I like this; can we have it on the x86 side too?
> +  BLOCK_COMMENT(err_msg("Begin %s", entry_name(ek)));
> +  BLOCK_COMMENT(err_msg("End %s", entry_name(ek)));

Ok.  I'll use { and } instead of begin end like the other messages too.

  BLOCK_COMMENT(err_msg("Entry %s {", entry_name(ek)));
  BLOCK_COMMENT(err_msg("} Entry %s", entry_name(ek)));

> 
> Did you consider making the temp argument to argument_address be mandatory?
> It seems tricky that it can kill its input.

Yes I found that a bit confusing to understand.  I'll make it explicit.

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

> 
> Reviewed.

Thanks!

tom

> 
> -- John