Thanks for the comments, I'll look into it later in the summer.
On Tue, 26 Jun 2012, Richard Guenther wrote:
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
--
Marc Glisse
