[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