Re: [Patch] PR 51938: extend ifcombine

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sat, Jun 23, 2012 at 7:18 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> 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).

I was more concerned about the behavior with NaNs in general where
x < y is not equal to x >= y.  Now combine_comparison handles only
the very specific case of logical and or or of two comparisons with the
same operands, you basically make it handle && ~ and || ~, too
(and ~ && ~ and ~ || ~), so it seems that optionally inverting the result
of the 2nd operand should be enough making the interface prettier
compared to the bitmask.

With the following change this should be installed separately - it's a
functional change already now

	/* If we changed the conditions that cause a trap, we lose.  */
 	if ((ltrap || rtrap) != trap)
+	  {
+	    /* Try the inverse condition.  */
+	    compcode = COMPCODE_TRUE - compcode;
+	    exchg = true;
+	    trap = (compcode & COMPCODE_UNORD) == 0
+		   && (compcode != COMPCODE_EQ)
+		   && (compcode != COMPCODE_ORD);
+	  }
+	if ((ltrap || rtrap) != trap)
 	  return NULL_TREE;
       }
...
+      ret = fold_build2_loc (loc, tcode, truth_type, ll_arg, lr_arg);
+      if (exchg)
+	ret = fold_build1_loc (loc, TRUTH_NOT_EXPR, truth_type, ret);

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

Well, for one add an argument to ifcombine_iforif/ifandif whether the 2nd op
is inverted and handle that appropriately instead of copying the functions.

I'd go for a single bool argument for the inversion everywhere, too, both and
and or are commutative and you can handle all cases with that form.

Thanks,
Richard.

> --
> Marc Glisse