RFR: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888 [v9]
Emanuel Peter
epeter at openjdk.org
Thu Jan 4 16:25:36 UTC 2024
On Wed, 3 Jan 2024 15:53:04 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 11 commits:
>
> - Merge branch 'master' into JDK-8319793
> - review
> - Revert "Update src/hotspot/share/opto/castnode.hpp"
>
> This reverts commit 356c91cca911ed486f9f87f3eff53ce21e1e3ec9.
> - Revert "Update src/hotspot/share/opto/memnode.hpp"
>
> This reverts commit bdb731ea562f314f44d327f7243ef5cf9ad40b2e.
> - review
> - Update src/hotspot/share/opto/memnode.hpp
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
> - Update src/hotspot/share/opto/castnode.hpp
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
> - Update src/hotspot/share/opto/castnode.cpp
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
> - Update src/hotspot/share/opto/ifnode.cpp
>
> Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
> - Merge branch 'master' into JDK-8319793
> - ... and 1 more: https://git.openjdk.org/jdk/compare/15519285...dbe3c4c1
@rwestrel thanks for all the work! Generally I'm very happy with the approach.
I mostly left suggestions for better comments and improved naming.
src/hotspot/share/opto/ifnode.cpp line 573:
> 571: // that these Loads/Casts do not float above any of the dominating checks (even when the lowest dominating check is
> 572: // later replaced by yet another dominating check), we need to pin them at the lowest dominating check.
> 573: proj->pin_array_loads(igvn);
`pin_array_loads` suggests we only care about `Load`. But the comment suggests otherwise.
I would also appreciate if the comment said why there are now multiple dependencies. Actually, the problem is that we **would** have multiple dependency, but we only have one dependency input we can set, hence forgetting about the others. Pinning makes sure that there is no bypassing of dependencies, right?
src/hotspot/share/opto/ifnode.cpp line 1501:
> 1499:
> 1500: //------------------------------dominated_by-----------------------------------
> 1501: Node* IfNode::dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool range_check_smearing) {
I suggest that you replace `range_check_smearing` with `pin_dependencies` or similar.
Basically say what it will do in this method, rather than what is the use case.
Then add a comment above what is a usecase, and more comments in the case where you call it with `true`.
Because the range-check-smearing is not happening here but outside.
src/hotspot/share/opto/ifnode.cpp line 1517:
> 1515: prev_dom = idom;
> 1516: }
> 1517:
Can you say what exactly this did, and why it is safe to remove?
src/hotspot/share/opto/ifnode.cpp line 1541:
> 1539: // control dependent nodes end up at the lowest/nearest dominating check in the graph. To ensure that these
> 1540: // Loads/Casts do not float above any of the dominating checks (even when the lowest dominating check is later
> 1541: // replaced by yet another dominating check), we need to pin them at the lowest dominating check.
I like this comment. A picture would be a really nice addition.
RC[0] -> true
...
RC[6] -> false
RC[0] -> true
...
RC[3] -> false
ctrl dependent node x, assuming array[3] is safe.
x is first dependent on RC[3], which is now smeared to RC[0] (the lower one) and RC[6]. Now we discover that the lower RC[0] is dominated by the upper one, and skip RC[6]. Now x is only dependent on RC[6], which is true, and does not first check RC[6], which it should check.
I suggest you move all of this to where the range-check-smearing happens.
src/hotspot/share/opto/ifnode.cpp line 1805:
> 1803: --i;
> 1804: }
> 1805: }
This logic looks like it would not just pin array loads, but really any node that has `depends_only_on_test`. That could also be `CastII` or even other nodes like `LoadKlass`, right? If that is true, you should rename this method to something more precise.
src/hotspot/share/opto/ifnode.cpp line 1958:
> 1956: return nullptr;
> 1957: }
> 1958:
Why is it ok to delay this to post-loop-opts? Does it not prevent moving some CFG from being eliminated out of loops? Would be nice to have a little justification comment.
src/hotspot/share/opto/loopopts.cpp line 308:
> 306: // IGVN worklist for later cleanup. Move control-dependent data Nodes on the
> 307: // live path up to the dominating control.
> 308: void PhaseIdealLoop::dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip, bool range_check_predicate) {
Can we also rename `range_check_predicate` -> `must_pin_dependencies`, so that it says what it does? And then add a comment to say that it is on when we are doing range check predication, and hence the eliminated RC lay between two predicates, and hence has a dependency on both.
src/hotspot/share/opto/loopopts.cpp line 356:
> 354: _igvn.replace_input_of(cd, 0, prevdom);
> 355: if (range_check_predicate) {
> 356: // Loads and range check Cast nodes that are control dependent on this range check (that is about to be removed)
Here we should now be talking about range check predicates, and not just range checks, right?
src/hotspot/share/opto/loopopts.cpp line 361:
> 359: return; // Let IGVN transformation change control dependence.
> 360: }
> 361:
Why it ok to remove this bailout?
src/hotspot/share/opto/memnode.cpp line 851:
> 849: return !Type::cmp( _type, ((LoadNode&)n)._type ) &&
> 850: _control_dependency == ((LoadNode&)n)._control_dependency &&
> 851: _mo == ((LoadNode&)n)._mo;
might look nicer if you cast `n` once -> `load` and then use that.
src/hotspot/share/opto/node.hpp line 1140:
> 1138: virtual Node* pin_for_array_access() const {
> 1139: return nullptr;
> 1140: }
Can you please add a comment, what this method is for?
-------------
Changes requested by epeter (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16886#pullrequestreview-1804393994
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441946602
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441920140
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441949862
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441930213
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441935806
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441953152
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441963806
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441959206
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441972024
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441979826
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1441895016
More information about the hotspot-compiler-dev
mailing list