Re: [RFA] Factor conversion out of COND_EXPR using match.pd pattern

[Richard Biener <richard.guenther@gmail.com>]

On Mon, Jun 29, 2015 at 7:51 PM, Jeff Law <law@redhat.com> wrote:
> On 06/01/2015 04:55 AM, Richard Biener wrote:
>>
>> On Sat, May 30, 2015 at 11:11 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> (only commenting on the technique, not on the transformation itself)
>>>
>>>> +(simplify
>>>> +  (cond @0 (convert @1) INTEGER_CST@2)
>>>> +  (if (INTEGRAL_TYPE_P (TREE_TYPE (@1))
>>>> +       && COMPARISON_CLASS_P (@0)
>>>
>>>
>>>
>>> If you add COMPARISON_CLASS_P to define_predicates, you could write:
>>> (cond COMPARISON_CLASS_P@0 (convert @1) INTEGER_CST@2)
>>
>>
>> But that would fail to match on GIMPLE, so I don't like either variant
>> as Jeffs relies on the awkward fact that on GIMPLE cond expr conditions
>> are GENERIC and yours wouldn't work.
>>
>> That said - for this kind of patterns testcases that exercise the patterns
>> on GIMPLE would be very appreciated.
>
> It may be the case that these patterns don't make a lot of sense on gimple
> and should be restricted to generic, at least with our current
> infrastructure.
>
> The problem is when we lower from generic to gimple, we end up with branchy
> code, not straight line code and there's no good way I see to write a
> match.pd pattern which encompasses flow control.
>
> So to discover the MIN/MAX with typecast, we're probably stuck hacking
> minmax_replacement to know when it can ignore the typecast.
>
> That may in fact be a good thing -- I haven't looked closely yet, but 45397
> may be of a similar nature (no good chance to see straight line code for
> match.pd, thus we're forced to handle it in phiopt).
>
>
> So do we want to restrict the new pattern on GENERIC, then rely on phiopt to
> get smarter and catch this stuff for GIMPLE?  Or drop the pattern totally
> and do everything in phiopt on GIMPLE?

Note that IMHO it doesn't make a lot of sense to add match.pd patterns
restricted
to GENERIC - those should simply go to / stay in fold-const.c.  For patterns
restricted to GIMPLE match.pd is still the proper place.

As for matching control flow it's actually not that difficult to get
it working at
least for toplevel COND_EXPRs.  The trick is to match on the PHI nodes
(like phiopt does), thus for

  if (cond)
...
  _3 = PHI <_4, _5>

ask the match-and-simplify machinery

  if (gimple_simplify (COND_EXPR, TREE_TYPE (_3), cond, _4, _5, ...))

which will then for example match

(simplify
 (cond (gt @0 @1) @0 @1)
 (max @0 @1))

for non-toplevel COND_EXPRs we'd need to adjust the matching code itself
to handle PHI defs.

Of course with this there are several complications arising.  One is cost
as the conditional might not go away (other code may be control dependet
on it).  One is SSA form - if you get complicated enough patterns you
might end up substituting a value into the result that is computed in a place
that does not dominate the PHI result (so you'd need to insert a PHI node
for it and figure out a value for the other edges ... which might mean such
patterns would be invalid anyway?).

So it's indeed not clear if re-writing phiopt to match.pd patterns is possible
or desirable.

>>
>>> or maybe use a for loop on comparisons, which would give names to
>>> TREE_OPERAND (@0, *). This should even handle the operand_equal_p
>>> alternative:
>>>
>>> (cond (cmp:c@0 @1 @2) (convert @1) INTEGER_CST@2)
>>
>>
>> Yes, that would be my reference.
>
> But won't this require pointer equivalence?  Are INTEGER_CST nodes fully
> shared?  What if @1 is something more complex than a _DECL node (remember,
> we're working with GENERIC).  So something like
> (cond (cmp:c@0 @1 @2) (convert @3) INTEGER_CST@4))
>
> And using operand_equal_p seems more appropriate to me (and is still better
> than the original (cond @0 ...) and grubbing around inside @0 to look at
> operands.

We do use operand_equal_p to query whether @0 and @0 are equal.

>>
>>>> +       && int_fits_type_p (@2, TREE_TYPE (@1))
>>>> +       && ((operand_equal_p (TREE_OPERAND (@0, 0), @2, 0)
>>>> +           && operand_equal_p (TREE_OPERAND (@0, 1), @1, 0))
>>>> +          || (operand_equal_p (TREE_OPERAND (@0, 0), @1, 0)
>>>> +              && operand_equal_p (TREE_OPERAND (@0, 1), @2, 0))))
>>>> +    (with { tree itype = TREE_TYPE (@1); tree otype = TREE_TYPE (@2); }
>>>> +      (convert:otype (cond:itype @0 @1 (convert:itype @2))))))
>>>
>>>
>>>
>>> This should be enough, no need to specify the outer type
>>> (convert (cond:itype @0 @1 (convert:itype @2))))))
>>
>>
>> Yes.
>>
>>> I believe we should not have to write cond:itype here, cond should be
>>> made
>>> to use the type of its second argument instead of the first one, by
>>> default
>>> (expr::gen_transform already has a few special cases).
>>
>>
>> Indeed.  Patch welcome (I'd have expected it already works...)
>
> With Marc's fix, those explicit types are no longer needed.

Good.

Richard.

> Jeff