[9] RFR (M): VM should constant fold Unsafe.get*() loads from final fields

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Wed Jun 24 12:59:13 UTC 2015


John, Paul, thanks for review!

Updated webrev:
   http://cr.openjdk.java.net/~vlivanov/8078629/webrev.01/

I spotted a bug when field and accessor types mismatch, but the JIT 
still constant-folds the load. The fix made expected result detection 
even more complex, so I decided to get rid of it & WhiteBox hooks 
altogether. The test exercises different code paths and compares 
returned values now.

> WB.isCompileConstant is a nice little thing.  We should consider using it in java.lang.invoke
> to gate aggressive object-folding optimizations.  That's one reason to consider putting it
> somewhere more central that WB.  I can't propose a good place yet.  (Unsafe is not quite right.)
Actually, there's already j.l.i.MethodHandleImpl.isCompileConstant.
Probably, compiler-specific interface is the right place for such 
things. But, as I wrote before, I decided to avoid WB hooks.

> The gating logic in library_call includes this extra term:  && alias_type->field()->is_constant()
> Why not just drop it and let make_constant do the test (which it does)?
I wanted to stress that make_constant depends on whether the field is 
constant or not. I failed to come up with a better method name 
(try_make_constant? make_constant_attempt), so I decided to keep the 
extra condition.

> You have some lines with "/*require_const=*/" in two places; that can't be right.
> This is the result of functions with too many misc. arguments to keep track of.
> I don't have the code under my fingers, so I'm just guessing, but here are more suggestions:
Thanks! I tried to address all your suggestions in updated version.

Best regards,
Vladimir Ivanov

>
> I wish the is_autobox_cache condition could be more localized.  Could we omit the boolean
> flag (almost always false), and where it is true, post-process the node?  Would that make
> the code simpler?
>
> This leads me to notice that make_constant is not related strongly to GraphKit; it is really
> a call to the Type and CI modules to look for a singleton type, ending with either a NULL
> or a call to GraphKit::makecon.  So you might consider changing Node* GK::make_constant
> to const Type* Type::make_constant.
>
> Now to pick at the argument salad we have in push_constant:  The effect of is_autobox_cache
> could be transferred to a method Type[Ary]::cast_to_autobox_cache(true).
> And the effect of stable_type on make_constant(ciCon,bool,bool,Type*),
> could also be factored out, as post-processing step contype=contype->Type::join(stabletype).




More information about the hotspot-compiler-dev mailing list