[lworld] RFR: 8262831: [lworld] C2 compilation fails with assert "should be addp but is Phi" [v2]
Yi Yang
yyang at openjdk.java.net
Mon Mar 29 14:42:06 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
Yi Yang has updated the pull request incrementally with one additional commit since the last revision:
syntax error
-------------
Changes:
- all: https://git.openjdk.java.net/valhalla/pull/371/files
- new: https://git.openjdk.java.net/valhalla/pull/371/files/57704eaf..0a5b6345
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=valhalla&pr=371&range=01
- incr: https://webrevs.openjdk.java.net/?repo=valhalla&pr=371&range=00-01
Stats: 1 line in 1 file changed: 0 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