This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch] PR 51938: extend ifcombine


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]