[lworld] RFR: 8262831: [lworld] C2 compilation fails with assert "should be addp but is Phi"

[Tobias Hartmann]

On Thu, 25 Mar 2021 08:55:40 GMT, Yi Yang <yyang at openjdk.org> wrote:

> 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

Looks good to me. Thanks for fixing.

src/hotspot/share/opto/memnode.cpp line 3926:

> 3924:                 Node* base = other_adr;
> 3925:                 if (base->is_Phi()) {
> 3926:                   // In rare case, base may be a PhiNode and it may reads

`it may reads` -> `it may read`

-------------

Marked as reviewed by thartmann (Committer).

PR: https://git.openjdk.java.net/valhalla/pull/371