RFR: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888 [v2]

Thu Dec 14 13:49:47 UTC 2023

On Wed, 13 Dec 2023 08:55:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Range check smearing and range check predication make an array access
>> dependent on 2 (or more in the case of RC smearing) conditions. As a
>> consequence, if a range check can be eliminated because there's an
>> identical dominating range check, the control dependent nodes that
>> could float and become dependent on the dominating range check cannot
>> be allowed to float because there's a risk that they would then bypass
>> one of the checks that make the access legal.
>> 
>> `IfNode::dominated_by()` and `PhaseIdealLoop::dominated_by()` have
>> logic to prevent this: nodes that are control dependent on a range
>> check or predicate are not allowed to float. This is however not
>> sufficient as demonstrated by the test cases.
>> 
>> In `TestArrayAccessAboveRCAfterSmearingOrPredication.testRangeCheckSmearing()`:
>> 
>> 
>>                 v += array[i];
>>                 if (flag2) {
>>                     if (flag3) {
>>                         field = 0x42;
>>                     }
>>                 }
>>                 if (flagField == 1) {
>>                     v += array[i];
>>                 }
>> 
>> 
>> The range check for the second `array[i]` load is replaced by the
>> dominating range check for the first `array[i]` but because the second
>> `array[i]` load could really be dependent on multiple range checks (in
>> case smearing happened which is not the case here), c2 doesn't allow
>> the second `array[i]` to float when the second range check is
>> removed. The second `array[i]` is then control dependent on:
>> 
>> 
>>                 if (flagField == 1) {
>> 
>> 
>> which is next found to be dominated by the same test:
>> 
>> 
>>         if (flag == 1) {
>> 
>> 
>> and is removed. However nothing in `dominated_by()` treats node
>> dependent on tests that are not range check or predicates
>> specially. So the second `array[i]` is allowed to float and become
>> dependent on:
>> 
>> 
>>         if (flag == 1) {
>> 
>> 
>> which is above the range check for that access. The test method in its
>> last invocation is passed an index for the array access that's widely
>> out of range. The array load happens before the range check and
>> crashes the VM. `testLoopPredication()` is a similar test where array
>> loads become dependent on predicates and end up above range checks.
>> 
>> `TestArrayAccessCastIIAboveRC.java` is the test case from the bug
>> where for similar reasons a range check `CastII` ends up above its
>> range check, becomes top because its input becomes some integer that
>> conflicts with its...
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into JDK-8319793
>  - fix & test

Nice summary! I have a few comments but otherwise, the fix looks reasonable. 

Have you also run some performance testing to check if delaying RC smearing has any impact?

src/hotspot/share/opto/castnode.cpp line 326:

> 324: #endif
> 325: 
> 326: CastIINode* CastIINode::pin_for_array_access() const {

Not sure if it's worth but you could sanity assert here that `_dependency == RegularDependency` since you always have checked `depends_only_on_test()` before calling this.

src/hotspot/share/opto/castnode.cpp line 328:

> 326: CastIINode* CastIINode::pin_for_array_access() const {
> 327:   if (has_range_check()) {
> 328:     return new CastIINode(in(0), in(1), bottom_type(), ConstraintCastNode::StrongDependency, has_range_check());

`ConstraintCastNode::` can be removed:
Suggestion:

    return new CastIINode(in(0), in(1), bottom_type(), StrongDependency, has_range_check());

src/hotspot/share/opto/castnode.hpp line 127:

> 125:   }
> 126: 
> 127:   CastIINode* pin_for_array_access() const;

You could add `override` to easier identify that this is an overriding method. Same in `LoadNode`.
Suggestion:

  CastIINode* pin_for_array_access() const override;

src/hotspot/share/opto/ifnode.cpp line 566:

> 564:   if (new_cmp == cmp) return;
> 565:   // Else, adjust existing check
> 566:   Node *new_bol = gvn->transform( new BoolNode(new_cmp, bol->as_Bool()->_test._test));

Suggestion:

  Node* new_bol = gvn->transform(new BoolNode(new_cmp, bol->as_Bool()->_test._test));

src/hotspot/share/opto/loopopts.cpp line 358:

> 356:         // Loads and range check Cast nodes that are control dependent on this range check depend on multiple dominating
> 357:         // range checks and can't float even if the range check they'll be control dependent on once this function
> 358:         // returns is replaced by a dominating range check: pin them.

I suggest to add some more details from the PR description (could be done analogously for the other two comment similar comments). How about:

// Loads and range check Cast nodes that are control dependent on this range check (that is about to be removed)
// now depend on multiple dominating range checks. After the removal of this range check, these control dependent
// nodes end up at the lowest/nearest dominating check in the graph. To ensure that these Loads/Casts do not float
// above any of the dominating checks (even when the lowest dominating check is later replaced by yet another
// dominating check), we need to pin them at the lowest dominating check.

src/hotspot/share/opto/memnode.hpp line 295:

> 293:   bool has_pinned_control_dependency() const   { return _control_dependency == Pinned; }
> 294: 
> 295:   LoadNode* pin_for_array_access() const;

Suggestion:

  LoadNode* pin_for_array_access() const override;

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16886#pullrequestreview-1781358495
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426522329
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426522454
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426480931
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426437097
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426736470
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1426481493