[Patch] PR 51938: extend ifcombine

[Marc Glisse]

Hello,

thanks for looking into the patch. A couple more details now that I am 
back from a conference:

On Wed, 20 Jun 2012, Marc Glisse wrote:

> On Wed, 20 Jun 2012, Richard Guenther wrote:
>
>> On Sun, Jun 10, 2012 at 4:16 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>> Hello,
>>> 
>>> currently, tree-ssa-ifcombine handles pairs of imbricated "if"s that share
>>> the same then branch, or the same else branch. There is no particular 
>>> reason
>>> why it couldn't also handle the case where the then branch of one is the
>>> else branch of the other, which is what I do here.
>>> 
>>> Any comments?
>> 
>> The general idea looks good, but I think the patch is too invasive.  As far
>> as I can see the only callers with a non-zero 'inv' argument come from
>> ifcombine_ifnotorif and ifcombine_ifnotandif (and both with inv == 2).

The idea of also supporting inv==1 or inv==3 is for uniformity, and 
because we might at some point want to get rid of the 'or' function and 
implement everything in terms of the 'and' version, with suitably inverted 
tests.

>> I would rather see a more localized patch that makes use of
>> invert_tree_comparison to perform the inversion on the call arguments
>> of maybe_fold_and/or_comparisons.

I would rather have done that as well, and as a matter of fact that is 
what the first version of the patch did, until I realized that 
-ftrapping-math was the default.

>> Is there any reason that would not work?
>
> invert_tree_comparison is useless for floating point (the case I am most 
> interested in) unless we specify -fno-trapping-math (writing this patch 
> taught me to add this flag to my default flags, but I can't expect everyone 
> to do the same). An issue is that gcc mixes the behaviors of qnan and snan 
> (it is not really an issue, it just means that !(comparison) can't be 
> represented as comparison2).
>
>> At least
>> 
>> +  if (inv & 1)
>> +    lcompcode2 = COMPCODE_TRUE - lcompcode2;
>> 
>> looks as if it were not semantically correct - you cannot simply invert
>> floating-point comparisons (see the restrictions invert_tree_comparison
>> has).
>
> I don't remember all details, but I specifically thought of that, and the 
> trapping behavior is handled a few lines below.

More specifically, it has (was already there, I didn't add it):
    if (!honor_nans)
...
    else if (flag_trapping_math)
      {
         /* Check that the original operation and the optimized ones will trap
            under the same condition.  */
         bool ltrap = (lcompcode & COMPCODE_UNORD) == 0
                      && (lcompcode != COMPCODE_EQ)
                      && (lcompcode != COMPCODE_ORD);
... same for rtrap and trap

         /* In a short-circuited boolean expression the LHS might be
            such that the RHS, if evaluated, will never trap.  For
            example, in ORD (x, y) && (x < y), we evaluate the RHS only
            if neither x nor y is NaN.  (This is a mixed blessing: for
            example, the expression above will never trap, hence
            optimizing it to x < y would be invalid).  */
         if ((code == TRUTH_ORIF_EXPR && (lcompcode & COMPCODE_UNORD))
             || (code == TRUTH_ANDIF_EXPR && !(lcompcode & COMPCODE_UNORD)))
           rtrap = false;

         /* If the comparison was short-circuited, and only the RHS
            trapped, we may now generate a spurious trap.  */
         if (rtrap && !ltrap
             && (code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR))
           return NULL_TREE;

         /* If we changed the conditions that cause a trap, we lose.  */
         if ((ltrap || rtrap) != trap)
...

which presumably ensures that the trapping behavior is preserved (I'll 
have to double-check that I didn't break that logic).

Do you have an idea how this can be handled in a less intrusive way (and 
without duplicating too much code)?

-- 
Marc Glisse