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

[Tobias Hartmann]

On Thu, 12 Jan 2023 22:23:39 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

>> But wouldn't that require duplication of the `ciEnv::check_stable_value` logic and having a `GrowableArray` per `ciObject` instance? 
>> 
>> 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 (since `null` is a valid value) and the lookup is done only once during (I)GVN. The regression test includes a corresponding case (see `testFinal`). Even with unsafe accesses, if the field holder is not known at parse time, the access is conservatively marked as mismatched and thus no constant folding is attempted later.
>
>> 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?

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

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