[9] RFR(M): 8038348: Instance field load is replaced by wrong data Phi

[Vladimir Kozlov]

Thank you Tobias for looking on this old problem.

So we have to find a data Phi which gives the same result as LoadNode::split_through_phi() for split through memory Phi3. Identity() code is short cut for that. Unfortunately I forgot what the case 
was when I added this code.
It is definitely incorrect in this case when you have multiple data Phi nodes.

Since in general case it will be difficult to find correct Phi may be we should not do this optimization. What happened if we do not do replacement if the region has several Phis? Can you check?

Also what happens with LoadNode::split_through_phi() call which should happened before calling Identity()?

Thanks,
Vladimir

On 7/29/16 4:42 AM, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8038348
> http://cr.openjdk.java.net/~thartmann/8038348/webrev.00/
>
> In C2, instance field loads may be replaced by a corresponding, already existing data Phi in LoadNode::identity():
>    // Search for an existing data phi which was generated before for the same
>    // instance's field to avoid infinite generation of phis in a loop.
>
> The problem is, that we only check if the Phi is from the same region and corresponds to the same instance field. We don't check if it contains the actual "up to date" value from the last store to the input memory:
>    if (phi->is_Phi() && phi != mem &&
>        phi->as_Phi()->is_same_inst_field(this_type, this_iid, this_index, this_offset)) {
>      return phi;
>    }
>
> In rare cases, it may happen that the same region contains multiple data Phis for the same instance field. For example:
>
>            AndI    AndI
>            /  \    / \
>           /  __\___| |
>           | |  |     |
>   Region  | |  AddI  AddI
>     |  \  | |  | |   |  \_________
>     |   \ | /  | |___|_____       |
>     |   Phi1   |     |     |      |
>     |________  |  ___|  Store1 Store2
>              \ | /          \   /
>               Phi2          Phi3
>                               |
>                             LoadI
>
> In this case, Phi3 is the instance field memory and Phi1/Phi2 are data Phis containing the value of the field. LoadINode::Identity() now tries to replace the load by a data Phi representing the field values corresponding to Store1 and Store2. Because Phi1Node::is_same_inst_field() returns true, the load is replaced by Phi1 which contains an "old" field value. We should use Phi2 instead.





>
> I added PhiNode::mem_contains_value() to check if the given 'value' Phi contains the values from stores on the corresponding input paths of this memory Phi by walking up the memory and data input chains and checking for consistency. To keep complexity low, the implementation is conservative:
> - If true, 'value' is guaranteed to contain the values from stores on the memory Phi input paths.
> - If false, either 'value' is not the correct Phi or we were not able to prove this due to optimizations of the memory chain that are not (yet) reflected by the value chain nodes or vice versa.
>
> This problem was caught by the Apache Lucene team some time ago [1] because it causes "impossible" assertions in one of their tests (see my investigation in the bug comments). I was able to reproduce the bug with an old version of their test suite (with JDK 7, 8 and 9) but unfortunately, I was not able to write a regression test. This is because the problem depends on the order in which nodes are being processed by IGVN (field info needs to propagate to Phis) and the order of nodes in the out array of the Region.
>
> I verified that the fix solves the problem reported by Apache Lucene with JDK 7, 8 and 9. I also executed the hs-comp RBT. Performance numbers look okay (still running).
>
> Thanks,
> Tobias
>
> [1] https://issues.apache.org/jira/browse/LUCENE-5168
>