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

[Vladimir Kozlov]

Hi Tobias,

In the head comment of RegionNode::optimize_trichotomy() add code examples you optimizing.

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

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.

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)
+  }

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.

Thanks,
Vladimir

On 9/26/18 1:25 AM, Tobias Hartmann wrote:
> Hi John,
> 
> thanks for looking at this again!
> 
> On 26.09.2018 01:57, John Rose wrote:
>> `res[9][9]` should be `res[illegal+1][illegal+1]` and should have rows and columns for `never`
>> (code smell:  `9` is a naked constant; makes it hard to tell your table is out of date)
> 
> Right, I've updated the table.
> 
>> In the test cases `compare1` has `(a < b) ? -1 : (a == b) ? 0 : 1`.
>> Shouldn’t you also test `(a < b) ? -1 : (a <= b) ? 0 : 1`?
>> And similarly, for other cases where the second test overlaps
>> with the first.
> 
> I did not add tests for all the 6² operator combinations but I think more overlapping tests won't
> hurt. I've added
> 
>   (a < b)  ? -1 : (a <= b) ?  0 :  1;
>   (a > b)  ?  1 : (a >= b) ?  0 : -1;
>   (a == b) ?  0 : (a <= b) ? -1 :  1;
>   (a == b) ?  0 : (a >= b) ?  1 : -1;
> 
> and verified that all inlined comparisons fold.
> 
> Here's the new webrev:
> http://cr.openjdk.java.net/~thartmann/8210215/webrev.01/
> 
> Thanks,
> Tobias
>