RFR (M): VM should constant fold Unsafe.get*() loads from final fields
vladimir.x.ivanov at oracle.com
Tue Jul 7 14:29:01 UTC 2015
Any volunteers to review updated version? Thanks!
On 6/24/15 3:59 PM, Vladimir Ivanov wrote:
> John, Paul, thanks for review!
> Updated webrev:
> 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: &&
>> 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*
>> 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
More information about the hotspot-compiler-dev