Request for reviews (XL): 6829193: JSR 292 needs to support SPARC

[John Rose]

On Apr 21, 2010, at 12:32 PM, John Rose wrote:

>> regcon_sll_ptr_by_con sure is convenient but its implementation is totally surprising and the name is long and unclear particularly since it has the same signature as regcon_sll_ptr but does something different.  It desperately needs a comment in the hpp explaining what it does.  I don't think dst should ever be optional, otherwise why it at all.  This use:
>> 
>> +      __ add(Gargs, __ regcon_sll_ptr_by_con(G5_stack_move, LogBytesPerWord, G5_stack_move), Gargs);
>> 
>> should just be sll_ptr.  Actually given all the magic that the existing regcon_sll_ptr does, couldn't you shoehorn the new behaviour into it?
> 
> I would prefer that, but will defer to others on what is "too much magic".

Looking in more detail, I think regcon_sll_ptr is already designed to do what regcon_sll_ptr_by_con is trying to do.  So I think you should get rid of the longer-named thing.  Here's an example of how to replace the longer function by the shorter, with code that is no more complicated and perhaps more readable:

diff --git a/src/cpu/sparc/vm/methodHandles_sparc.cpp b/src/cpu/sparc/vm/methodHandles_sparc.cpp
--- a/src/cpu/sparc/vm/methodHandles_sparc.cpp
+++ b/src/cpu/sparc/vm/methodHandles_sparc.cpp
@@ -171,7 +171,8 @@
   //   for (temp = sp + size; temp < argslot; temp++)
   //     temp[-size] = temp[0]
   //   argslot -= size;
-  RegisterOrConstant offset = __ regcon_sll_ptr_by_con(arg_slots, LogBytesPerWord, temp3_reg);
+  RegisterOrConstant offset = arg_slots;
+  __ regcon_sll_ptr(offset, LogBytesPerWord, temp3_reg);
   // Keep the stack pointer 2*wordSize aligned.
   const int TwoWordAlignmentMask = ((1 << LogBytesPerWord) * 2) - 1;
   if (offset.is_register()) {

Although I like the way the code reads, the side-effect to &offset is a little unnerving when you look into the details.  I could live with reformulating the regcon_foo_ptr macros to return the new RegisterOrConstant value as an explicit result, rather than patching it in-place as the dest argument.

But I think regcon_sll_ptr_by_con should go.

-- John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20100421/f89046e1/attachment.html