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

[Vladimir Ivanov]

Thanks, Vladimir!

Empty files are a webrev artifact when it works with a set of patches 
containing negating changes.

Best regards,
Vladimir Ivanov

On 7/7/15 8:48 PM, Vladimir Kozlov wrote:
> Looks reasonable to me.
>
> graphKit.* files are listed without changes.
>
> Thanks,
> Vladimir K
>
> On 7/7/15 7:29 AM, Vladimir Ivanov wrote:
>> Any volunteers to review updated version? Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 6/24/15 3:59 PM, Vladimir Ivanov wrote:
>>> 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).
>>>
>>>