review for 7045514: SPARC assembly code for JSR 292 ricochet frames
Tom Rodriguez
tom.rodriguez at oracle.com
Thu Jun 2 12:21:50 PDT 2011
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
More information about the hotspot-compiler-dev
mailing list