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

[John Rose]

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

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*/)

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

(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?

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.

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.

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

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

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.

Reviewed.

-- John