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

[Vladimir Kozlov]

Hi Tobias,

On 10/1/18 2:17 AM, Tobias Hartmann wrote:
> 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.

We already have this optimization (for no Phi nodes) in RegionNode::Ideal() [1]:

http://hg.openjdk.java.net/jdk/jdk/file/2f1698b6db15/src/hotspot/share/opto/cfgnode.cpp#l432

I don't think we need to duplicate it here - just return false. Did you found that it does not work 
in your case?

Thanks,
Vladimir

> 
>> 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
>