RFR: 8295486: Inconsistent constant field values observed during compilation

[Jatin Bhateja]

On Thu, 5 Jan 2023 13:26:11 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> We hit a "not monotonic" assert because the new type of a load from a stable final field is more narrow than the old type which contradicts the assumption that types should only go from TOP to BOTTOM during CCP:
> 
> old: `narrowoop: java/lang/Integer:BotPTR:exact *`
> new: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...`
> 
> or 
> 
> old: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e538} ...`
> new: `narrowoop: java/lang/Integer java.lang.Integer {0x000000062c41e548} ...`
> 
> The problem is that a stable field can be (re-)initialized during compilation and since the value is not cached, contradicting types can be observed. In `LoadNode::Value`, we re-read the field value each time: 
> 
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/memnode.cpp#L1994-L1997
> 
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/type.cpp#L332-L337
> 
> The same problem exists for loads from stable arrays:
> https://github.com/openjdk/jdk/blob/872384707e89d03ede655aad16f360dc94f10152/src/hotspot/share/opto/memnode.cpp#L1923
> 
> Caching the field value is not feasible as it would require a cache per ciInstance for all the fields and per ciArray for all the elements. Alternatively, we could keep track of the lookup and only do it once but that would also be lots of additional complexity for a benign issue.
> 
> Instead, I propose to skip verification during CCP when folding loads from stable fields. Non-stable, constant fields are not affected as `null` is a valid value for them and they would already be folded before CCP.
> 
> Thanks,
> Tobias

> > The definition of @stable is basically that we can constant-fold any value once it's initialized and (semantically) it does not make a difference which value we choose.
> 
> Okay. The spec is really bizarre for @stable
> 
> > If we go with your proposed solution of a table of field values, we can as well use that one as cache instead of bailing out from compilation, right?
> 
> Right. I prefer caching because the value will be consistent at least during compilation and it may allow some optimizations to happen. Imaging you read same stable field on separate paths and then compare constants.

If value keep changing during CCP then it will delay the convergence. Thus a convergence will be reached only for a stable value,  which is what caching will also achieve. Stable field value should be added to method dependencies such that compilation becomes invalid if stable value changes later on.

Agree with @iwanowww suggestion

-------------

PR: https://git.openjdk.org/jdk/pull/11861