RFR: 8295486: Inconsistent constant field values observed during compilation [v5]

[Tobias Hartmann]

On Fri, 20 Jan 2023 08:09:52 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
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix to verification code

Thanks, Vladimir! I updated the code according to your suggestions.

> As it is now, it doesn't help static/instance fields, but it can be improved. Something for a follow-up cleanup.

Could you elaborate? Final static/instance fields are handled as well (`ciField::constant_value_of` -> `ciInstance::field_value` -> `ciInstance::field_value_impl`). Do you mean non-final fields?

> Testing now revealed a new "Not monotonic" assert that seems to be triggered by caching non-stable final fields. I'll investigate but I'm not able to reproduce it yet.

I reproduced it. The problem is the new call to `verify_type` that I added to `PhaseCCP::verify_analyze`. It's executed before the `LoadNode` bailout:


        // MemNode::can_see_stored_value looks up through many memory nodes,
        // which means we would need to notify modifications from far up in
        // the inputs all the way down to the LoadNode. We don't do that.


I moved it below the bailout. I think we can also strengthen the verification by only bailing out if the current type is not a singleton, because even for LoadNodes, it should never happen that a node with a singleton type is updated to a different type.

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

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