[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.

![crash1](https://user-images.githubusercontent.com/5010047/112446079-c977b180-8d8a-11eb-9d75-3c0dd7487e58.jpg)

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.

![crash2](https://user-images.githubusercontent.com/5010047/112446126-d5637380-8d8a-11eb-834e-7b62d03acd25.jpg)

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