On Sat, Jun 23, 2012 at 7:18 PM, Marc Glisse <marc.gli...@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.gli...@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