RFR: 8330157: C2: Add a stress flag for bailouts [v13]

[Christian Hagedorn]

On Fri, 4 Oct 2024 06:30:11 GMT, Daniel Skantz <duke at openjdk.org> wrote:

>> This patch adds a diagnostic/stress flag for C2 bailouts. It can be used to support testing of existing bailouts to prevent issues like [JDK-8318445](https://bugs.openjdk.org/browse/JDK-8318445), and can test for issues only seen at runtime such as [JDK-8326376](https://bugs.openjdk.org/browse/JDK-8326376). It can also be useful if we want to add more bailouts ([JDK-8318900](https://bugs.openjdk.org/browse/JDK-8318900)).
>> 
>> We check two invariants.
>> a) Bailouts should be successful starting from any given `failing()` check.
>> b) The VM should not record a bailout when one is pending (in which case we have continued to optimize for too long).
>> 
>> a), b) are checked by randomly starting a bailout at calls to `failing()` with a user-given probability.
>> 
>> The added flag should not have any effect in debug mode.
>> 
>> Testing:
>> 
>> T1-5, with flag and without it. We want to check that this does not cause any test failures without the flag set, and no unexpected failures with it. Tests failing because of timeout or because an error is printed to output when compilation fails can be expected in some cases.
>
> Daniel Skantz has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use failing_internal instead; add a const; clarify skip

Thanks for the update, looks good! Some minor code style comments regarding existing code that you touched which I think you could also fix while at it.

src/hotspot/share/opto/compile.cpp line 4375:

> 4373: 
> 4374: Compile::TracePhase::~TracePhase() {
> 4375:   if (_compile->failing_internal()) return; // timing code, not stressing bailouts.

While at it, I suggest to add braces:

Suggestion:

  if (_compile->failing_internal()) {
    return; // timing code, not stressing bailouts.
  }

Same below at some places.

src/hotspot/share/opto/graphKit.cpp line 343:

> 341: // regions do not appear except in this function, and in use_exception_state.
> 342: void GraphKit::combine_exception_states(SafePointNode* ex_map, SafePointNode* phi_map) {
> 343:   if (failing_internal()) return;  // dying anyway...

Suggestion:

  if (failing_internal()) {
    return;  // dying anyway...
  }

src/hotspot/share/opto/graphKit.cpp line 2059:

> 2057:                              bool must_throw,
> 2058:                              bool keep_exact_action) {
> 2059:   if (failing_internal())  stop();

Suggestion:

  if (failing_internal()) {
    stop();
  }

src/hotspot/share/opto/loopnode.cpp line 4938:

> 4936: 
> 4937:   PhaseIdealLoop phase_verify(_igvn, this);
> 4938:   if (C->failing_internal()) return;

Suggestion:

  if (C->failing_internal()) {
    return;
  }

src/hotspot/share/opto/output.cpp line 3394:

> 3392: 
> 3393:   // Emitting into the scratch buffer should not fail
> 3394:   assert (!C->failing_internal() || C->failure_is_artificial(), "Must not have pending failure. Reason is: %s", C->failure_reason());

Suggestion:

  assert(!C->failing_internal() || C->failure_is_artificial(), "Must not have pending failure. Reason is: %s", C->failure_reason());

src/hotspot/share/opto/parse.hpp line 429:

> 427: 
> 428:   // Must this parse be aborted?
> 429:   bool failing()                { return C->failing_internal(); } // might have cascading effects, not stressing bailouts for now.

Can be made const:
Suggestion:

  bool failing() const { return C->failing_internal(); } // might have cascading effects, not stressing bailouts for now.

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19646#pullrequestreview-2350875835
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789512799
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789513200
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789513401
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789513596
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789513864
PR Review Comment: https://git.openjdk.org/jdk/pull/19646#discussion_r1789512066