[lworld] RFR: 8262831: [lworld] C2 compilation fails with assert "should be addp but is Phi"
Yi Yang
yyang at openjdk.java.net
Thu Mar 25 09:02:13 UTC 2021
Hi,
Can I have a review of this bug fix?
The following test case is crashed at `(5)`:
void test(MyValue[] array) {
for (int i = 0; i < 10; ++i) {
for (int j = 0; j < 10; ++j) { (1)
iField = 6; (2)
} (3)
for (int j = 0; j < 2; ++j) {
iField += array[0].b;
}
MyValue[] array2 = {new MyValue()}; (4)
c = array[0]; (5) // hit the assertion
array2[0] = t; (6)
}
}
I did some investigations. C2 wants to check whether there are other Mem nodes between `(4)` and `(6)` that read or write the array2, because it hopes to merge `(4)` and `(6)` into an InitializeNode. If it finds that there are any reads or writes, such as LoadI in `(5)`, then its Address input must be an AddPNode. But in fact, it may be a PhiNode, so an assertion is hit.

Why does PhiNode appear on (5) as the input of LoadINode? Because the loop unrolling (PhaseIdealLoop::do_unroll) occurred in `(1)-(3)`, it produced a cloned node of the parameter array(not as straightforward as I said, actually it's a CastPPNode which produced via extra steps), and then the parameter array and the cloned nodes were merged, thus a PhiNode node appeared.

Thanks Tobias for pointing out that this scenario is not reproducible in mainline JDK because we can't perform such aggressive scalarization for non-inline types. So we'd better fix it in Valhalla for now.
Thanks!
Yang
-------------
Commit messages:
- fix c2 crash
Changes: https://git.openjdk.java.net/valhalla/pull/371/files
Webrev: https://webrevs.openjdk.java.net/?repo=valhalla&pr=371&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8262831
Stats: 31 lines in 2 files changed: 30 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/valhalla/pull/371.diff
Fetch: git fetch https://git.openjdk.java.net/valhalla pull/371/head:pull/371
PR: https://git.openjdk.java.net/valhalla/pull/371
More information about the valhalla-dev
mailing list