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

[David Leopoldseder]

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

I did not consider the substrate runtime compilation use case  - that may actually lead to the same error of inconsistency we have seen as here. Probably not relevant now but if it ever pops up we need to relax the invariant on the graal side then.

Regarding doc changes - what is our final call now ? (a) drop all new doc again or (b) keep (whatever) form of the new doc I added? - Its hotspot specific so not wrong.

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

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