[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-graal-dev
Subject:    Re: RFR: 8322636: [JVMCI] HotSpotSpeculationLog can be inconsistent across a single compile
From:       Doug Simon <dnsimon () openjdk ! org>
Date:       2023-12-23 4:19:41
Message-ID: oGS-YsVfBzIP0dFhykKxU73CVj5oM9A7wzbnu5v4I-0=.d6560108-e53a-4681-8758-9169b570f9a9 () github ! com
[Download RAW message or body]

On Fri, 22 Dec 2023 09:55:16 GMT, David Leopoldseder <davleopo@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 during
  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`.

I think it's worth updating the javadoc for maySpeculate to clarify that it returns \
consistent results for any given speculation for the lifetime of a SpeculationLog \
object.

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

Marked as reviewed by dnsimon (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17183#pullrequestreview-1795390189


[prev in list] [next in list] [prev in thread] [next in thread]