for review (XXL): 6655638 method handles for invokedynamic

[John Rose]

On Mar 2, 2009, at 5:34 PM, Vladimir Kozlov wrote:

> Since delayed_value implementation can be safely separated
> from the rest of your changes can you push it separately?
> Here is review for it. In general it is good.
>
> -------------------
> Why you did not put class RegisterConstant into src/share/vm/asm/ 
> assembler.hpp?
> You can leave a comment where to look for it.

Good suggestion; this avoids silly code duplication.

I wanted to move up the definition of Register, which is the same text  
on all architectures, but was afraid to perturb other ports, just for  
one little change.  This would be a good thing to roll into a Steve G.  
type of refactoring cleanup.

> -------------------
> MacroAssembler::delayed_value() uses RegisterConstant constructors
> implicitly. Can you change it to explicit usage?

Good idea.  Done.  (Although I thought it was cute.  But cute doesn't  
pay for obscure.)

> -------------------
> assembler_x86.cpp/MacroAssembler::load_sized_value()
>
> And you asked Christian don't use high/low words :) :
>
> 6251   // For case 8, caller is responsible for manually loading
> 6252   // the high word into another register.

Heh.  s/high/second/.  Thanks.

> Also why you are using movptr() in the corresponding part in
> MethodHandles::generate_method_handle_stub() ?:
>
> 494         __ load_sized_value(rbx_temp, prim_value_addr,
> 495                             type2aelembytes(arg_type),  
> is_signed_subword_type(arg_type));
> 496         __ movptr(Address(rax_argslot, 0), rbx_temp);
> 497 #ifndef _LP64
> 498         if (arg_slots == 2) {
> 499           __ movl(rbx_temp, prim_value_addr.plus_disp(wordSize));
> 500           __ movptr(Address(rax_argslot,  
> Interpreter::stackElementSize()), rbx_temp);
> 501         }
> 502 #endif //_LP64

The movptr on line 500 looks better as movl; changed.
The movptr on line 496 is needed because interpreter stack slots are  
wordSize not jintSize.
Does that answer your concern?

> And may be better use load_signed_short/load_unsigned_short names:
>
> 6261   case ~2:  load_signed_word(   dst, src ); break;
> 6262   case  2:  load_unsigned_word( dst, src ); break;

You know what, I'm going to fix that now.  It's just confusing, and  
somebody has already left a comment to that effect in the code.  It  
was called "word" because that's how the Intel assembler manual talks  
about 16-bit values, as in "movswl".  But the term "word" in an  
identifier like "load_signed_word" is way out of context; no other  
part of assembler_x86.hpp uses "word" to mean 16 bits, and the term  
means "machine word size 32/64 bits"  everywhere else in HotSpot.

> -------------------
> assembler.cpp:
>
> We talked about to comment why DC_LIMIT = 20.

Those comments are in now.

> -------------------
> assembler.hpp:
>
> I think, instead of "// Use sparingly" it may be better to explain
> that is mostly used for java classes non-static field offsets
> (currently only for MethodHandle) when we need to generated assembler
> code before the class is loaded and field offsets are not known.
> Also that is why the value can't be 0 since an object has markword
> at 0 offset.
>
> 285   // The value zero (NULL) stands instead of a constant which is  
> still uncomputed.
> 286   // Use sparingly.

Done.

> -------------------
> Why the next method does not return intptr_t* also?
>
> static address*  delayed_value_addr(address(*constant_fn)());

No good reason; I made them the same, and it simplified the code.

> -------------------
> The variable last_WrongMethodType_caller is declared but is not used:
> src/cpu/sparc/vm/interpreter_sparc.cpp

Fixed.

> -------------------
> On x64 in 2 files in TemplateTable::prepare_invoke()
> why you changed movptr(recv  to  movl(recv / movq(recv ?
> Did you fix a problem? It should be separate bug in that case?

Good catch.  I think that's the residue from some search/replace  
experiments I was doing.  I removed the change.

> -------------------

Thanks!

-- John