RFR: 8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile [v2]

[Tom Rodriguez]

On Tue, 2 Jan 2024 13:37:19 GMT, David Leopoldseder <davleopo at openjdk.org> wrote:

>> This PR fixes a subtle inconsistency in `HotSpotSpeculationLog` .
>> 
>> Normal uses of `HotSpotSpeculationLog` work by using a `SpeculationReason` and asking the speculation log via `maySpeculate` if the speculation can be performed, i.e., if it failed before for the given method.  An example for this can be seen in Graal https://github.com/oracle/graal/blob/master/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/CountedLoopInfo.java#L591C15-L591C15 
>> The implicit assumption is that the speculation log, `HotSpotSpeculationLog` in particular collects failed speculations at the beginning of a compile and then stays consistent during the compile. Why is that? - Because if there are new failed speculations added to the failed speculations during the compile - the compiler would speculate again on those in an inconsistent way. E.g. at the beginning of a compile a certain speculation has not failed yet and the compiler thinks it can do optimization xyz using a speculation  - later during the compilation process it consults the speculation log but gets a different answer. All those inconsistent speculations that already failed will anyway later fail code installation in jvmci (they will throw a bailout during `HotSpotCodeCacheProvider#installCode` https://github.com/openjdk/jdk/blob/master/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotSpeculationLog.java#L192 ). Thus, we should at least return a consistent result durin
 g a compile.
>> The problem for consistency here, that also makes troubles on the graal side, is that `maySpeculate` itself can collect failed speculations if there have not been any previously, i.e., `failedSpeculations == null`.
>> In order to make the speculation log consistent across an entire JVMCI compile this PR removes the collection of failed speculations in `maySpeculate`.
>
> David Leopoldseder has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8322636: [JVMCI] HotSpotSpeculationLog add javadoc to maySpeculate

So I looked more closely the HotSpot and substrate implementations and I'm not sure we can currently align the implementation and the javadoc.  In the HotSpot world, HotSpotSpeculationLog is a compiler local object that reads data from the real speculation data that's kept in the MDO.  This means that it has full control over when collectFailedSpeculations is called.  SubstrateSpeculationLog is the actual log so if two threads are operating on the same log then one of them could see the effects of a call to collectFailedSpeculations by the other thread.  Maybe in practice 2 threads never do this because it would mean they are compiling the same root method but it doesn't seem guaranteed.  installCode on substrate also doesn't perform the speculation log check that HotSpot does.  So maybe we punt on javadoc updates for now.

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

PR Comment: https://git.openjdk.org/jdk/pull/17183#issuecomment-1875942561