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

[Vladimir Ivanov]

On Tue, 24 Jan 2023 13:51:28 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8295486
>  - Check cache before state transition
>  - Fix to verification code
>  - NULL -> nullptr
>  - Moved caching logic to ciObject
>  - Caching of stable values
>  - Updated copyright
>  - 8295486: Inconsistent constant field values observed during compilation

Overall, looks good!

src/hotspot/share/ci/ciObject.cpp line 194:

> 192: // Add a constant value to the cache.
> 193: void ciObject::add_to_constant_value_cache(int off, ciConstant val) {
> 194:   assert(val.is_valid(), "value must be valid");

Maybe add an assert to ensure the list doesn't contain the value?

assert(check_constant_value_cache(off, val.basic_type()).is_valid() == false, "duplicate");

src/hotspot/share/ci/ciObject.cpp line 197:

> 195:   if (_constant_values == nullptr) {
> 196:     Arena* arena = CURRENT_ENV->arena();
> 197:     _constant_values = new (arena) GrowableArray<ConstantValue>(arena, 0, 0, ConstantValue());

Considering an element is added right away, it makes sense to increase initial capacity (to 1, at least).

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

Marked as reviewed by vlivanov (Reviewer).

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