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

More specifically it only validates against the speculations that failed before the last call to collectFailedSpeculations which must always be called explicitly.  And we should point out somewhere that installCode will call collectFailedSpeculations before installation and revalidate the current set of speculations, bailing out if any were violated during compilation.  This doesn't seem to be documented anywhere.

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

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