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

[Vladimir Ivanov]

On Mon, 16 Jan 2023 09:44:21 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>>> But wouldn't that require duplication of the ciEnv::check_stable_value logic and having a GrowableArray per ciObject instance?
>> 
>> Are you primarily concerned about footprint? The field can be lazily initialized to reduce possible effects.
>> `check_stable_value` still can be shared (e.g., placed on `ciObject`).
>> 
>>> Regarding covering more cases like static final fields: I intentionally omitted them because I don't think it's possible to delay constant folding of such loads until CCP
>> 
>> If you want to limit the current patch to stable case only, that's fine with me. But keep in mind that we still want to uniformly treat all cases where field/array element values are considered constant. Concurrent field modifications introduce non-determinism into compilation process which we would like to avoid. So, even if we don't cover final fields right away, let's prepare for a future enhancement which will cover that. In that respect, stable references in names are misleading.
>
> My concern is that putting the logic in `ciObject` adds a new field to all subclass instances while it's only rarely needed. Given that a large number of these objects are created during compilation, that adds to footprint even if the cache is initialized lazily.
> 
> I still gave this a try and another issue is that using `ciConstant` in `ciObject` creates a circular dependency (`invalid use of incomplete type`) because `ciConstant` depends on `ciObject`. We could of course duplicate the logic and put things both into `ciInstance` and `ciArray` but then a side table in `ciEnv` seems preferable to me.
> 
> Until proven otherwise, I would like to limit the current patch to stable cases because I don't think there are any issues for non-stable cases. I can rename the code from "stable" to "constant" but I'm not sure how much sense that makes given it's all limited to and guarded by `FoldStableValues`. What do you think?

Personally, I'm not concerned about footprint increase from a field on `ciObject` class. Those represent oop constants observed by JITs and their count should be modest.

Speaking of relation between `ciConstant` and `ciObject`, `ciConstant.hpp` only refers to `ciObject*`, so the cycle can be broken by prepending `class ciObject;` in `ciConstant.hpp`. 

>  I don't think there are any issues for non-stable cases.
I'm fine with addressing only stable cases with the current patch for now, but I don't agree there's nothing to improve for non-stable cases.

Concurrent field modifications may seem benign, but our previous experience shows that it's safer to work with a consistent view through CI (when it is feasible/possible). 
(There's a relevant RFE filed: https://bugs.openjdk.org/browse/JDK-8294616) And it helps improve compilation replay along the way. And it's not specific to C2.

So, IMO in the longer term CI is the right place to fix it (irrespective whether stable or all constant cases are handled). It's a bit strange to see the new checks added in `opto/type.cpp` (irrespective of where the cache is located).

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

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