[12] RFR(M): 8210215: C2 should optimize trichotomy calculations

[Tobias Hartmann]

Hi Vladimir,

thanks for the thorough review!

On 26.09.2018 21:25, Vladimir Kozlov wrote:
> In the head comment of RegionNode::optimize_trichotomy() add code examples you optimizing.

Done.

> I think you need to return bool value if modification happens and || with 'modified' value.

Right but only if we modify "this". Done.

> In shape 1 check you check for region->outcnt() != 2. Does it mean this Region node does not have
> Phi node attached? 1 - is itself, 2 - is this Region node.

Yes, exactly. The "upper" region of shape 1 does not have any phis attached but only feeds into the
"this" region. That basically means that it makes no difference if we come from either control
input, the resulting control/value will be the same. I've updated the comment to make that clear.

> Next optimization seems wrong for case 1 where you don't check Phi inputs - it could be normal
> diamond shape with different Phi node values on each branch:
> 
> +  if (iff1 == iff2) {
> +    igvn->replace_input_of(region, idx1, iff1->in(0));
> +    igvn->replace_input_of(region, idx2, igvn->C->top());
> +    return; // Remove useless if (both projections map to the same control/value)
> +  }

No because the region for shape 1 does not have any Phi users. The graph would look like this when
above code is triggered:

     If
    /  \
 Proj  Proj
   \    /
   Region1   [...]
        \     /
        Region2

Region1 has no other (Phi) users and therefore the If is useless because both projections map to the
same control flow.

For shape 2, it would look like this:

     If
    /  \
 Proj  Proj
   \    /  [...]
    \  /   /
    Region1

Again, the if is useless because we know that the unique Phi user of region1 has the same inputs for
the two projection paths idx1 and idx2 coming from the if.

> I think you need to check control flow and Phi inputs for both cases to make sure you got only
> expected shapes before transforming graph.

I've added a comment that clarifies what guarantees we have after the initial checks. I don't think
additional checks are required.

Here's the new webrev:
http://cr.openjdk.java.net/~thartmann/8210215/webrev.02/

Thanks,
Tobias