Re: [PATCH] Fix PR77826
On Tue, 4 Oct 2016, Richard Biener wrote:
>
> The following will fix PR77826, the issue that in match.pd matching
> up two things uses operand_equal_p which is too lax about the type
> of the toplevel entity (at least for integer constants).
>
> Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
The following is what I have applied.
Richard.
2016-10-05 Richard Biener
PR middle-end/77826
* genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
with types_match for GIMPLE code gen to handle type mismatched
constants properly.
(dt_operand::gen): Adjust.
* match.pd ((X /[ex] A) * A -> X): Properly handle converted
and constant A.
* gcc.dg/torture/pr77826.c: New testcase.
Index: gcc/testsuite/gcc.dg/torture/pr77826.c
===
--- gcc/testsuite/gcc.dg/torture/pr77826.c (revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr77826.c (working copy)
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+
+void
+fi(unsigned long int *v0, unsigned int ow, int q2)
+{
+ if (ow + q2 != 0)
+if (q2 == 1)
+ {
+ *v0 |= q2;
+ q2 ^= *v0;
+ }
+}
Index: gcc/genmatch.c
===
--- gcc/genmatch.c (revision 240744)
+++ gcc/genmatch.c (working copy)
@@ -1562,7 +1562,7 @@ struct dt_operand : public dt_node
void gen (FILE *, int, bool);
unsigned gen_predicate (FILE *, int, const char *, bool);
- unsigned gen_match_op (FILE *, int, const char *);
+ unsigned gen_match_op (FILE *, int, const char *, bool);
unsigned gen_gimple_expr (FILE *, int);
unsigned gen_generic_expr (FILE *, int, const char *);
@@ -2589,12 +2589,18 @@ dt_operand::gen_predicate (FILE *f, int
a capture-match. */
unsigned
-dt_operand::gen_match_op (FILE *f, int indent, const char *opname)
+dt_operand::gen_match_op (FILE *f, int indent, const char *opname, bool gimple)
{
char match_opname[20];
match_dop->get_name (match_opname);
- fprintf_indent (f, indent, "if (%s == %s || operand_equal_p (%s, %s, 0))\n",
- opname, match_opname, opname, match_opname);
+ if (gimple)
+fprintf_indent (f, indent, "if (%s == %s || (operand_equal_p (%s, %s, 0) "
+ "&& types_match (%s, %s)))\n",
+ opname, match_opname, opname, match_opname,
+ opname, match_opname);
+ else
+fprintf_indent (f, indent, "if (%s == %s || operand_equal_p (%s, %s,
0))\n",
+ opname, match_opname, opname, match_opname);
fprintf_indent (f, indent + 2, "{\n");
return 1;
}
@@ -2991,7 +2997,7 @@ dt_operand::gen (FILE *f, int indent, bo
else if (type == DT_TRUE)
;
else if (type == DT_MATCH)
-n_braces = gen_match_op (f, indent, opname);
+n_braces = gen_match_op (f, indent, opname, gimple);
else
gcc_unreachable ();
Index: gcc/match.pd
===
--- gcc/match.pd(revision 240770)
+++ gcc/match.pd(working copy)
@@ -1773,9 +1803,13 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
/* (X /[ex] A) * A -> X. */
(simplify
- (mult (convert? (exact_div @0 @1)) @1)
- /* Look through a sign-changing conversion. */
- (convert @0))
+ (mult (convert1? (exact_div @0 @1)) (convert2? @2))
+ /* We cannot use matching captures here, since in the case of
+ constants we don't see the second conversion. Look through
+ a sign-changing or widening conversions. */
+ (if (operand_equal_p (@1, @2, 0)
+ && element_precision (@0) <= element_precision (type))
+ (convert @0)))
/* Canonicalization of binary operations. */
Re: [PATCH] Fix PR77826
On Wed, 5 Oct 2016, Richard Biener wrote:
The following will fix PR77826, the issue that in match.pd matching
up two things uses operand_equal_p which is too lax about the type
of the toplevel entity (at least for integer constants).
Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
The following is what I have applied.
Richard.
2016-10-05 Richard Biener
PR middle-end/77826
* genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
with types_match for GIMPLE code gen to handle type mismatched
constants properly.
I don't understand the disparity between generic and gimple here. Why let
(char)1 and (long)1 match in generic but not in gimple? And there are
probably several transformations in match.pd that could do with an update
if constants don't match anymore. Or did I misunderstand what the patch
does?
* match.pd ((X /[ex] A) * A -> X): Properly handle converted
and constant A.
This regressed
int f(int*a,int*b){return 4*(int)(b-a);}
--
Marc Glisse
Re: [PATCH] Fix PR77826
On Thu, 6 Oct 2016, Marc Glisse wrote:
> On Wed, 5 Oct 2016, Richard Biener wrote:
>
> > > The following will fix PR77826, the issue that in match.pd matching
> > > up two things uses operand_equal_p which is too lax about the type
> > > of the toplevel entity (at least for integer constants).
> > >
> > > Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
> >
> > The following is what I have applied.
> >
> > Richard.
> >
> > 2016-10-05 Richard Biener
> >
> > PR middle-end/77826
> > * genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
> > with types_match for GIMPLE code gen to handle type mismatched
> > constants properly.
>
> I don't understand the disparity between generic and gimple here. Why let
> (char)1 and (long)1 match in generic but not in gimple? And there are probably
> several transformations in match.pd that could do with an update if constants
> don't match anymore. Or did I misunderstand what the patch does?
The disparity is mostly that with GENERIC unfolded trees such as (char)1
are a bug while in GIMPLE the fact that the match.pd machinery does
valueization makes those a "feature" we have to deal with. Originally
I've restricted GENERIC as well but with its types_match_p implementation
it resulted in too many missed matches.
I agree that some transforms would need updates - I've actually tried
to implement a warning for genmatch whenever seeing a match with
(T)@0 but there isn't any good existing place to sneak that in.
> > * match.pd ((X /[ex] A) * A -> X): Properly handle converted
> > and constant A.
>
> This regressed
> int f(int*a,int*b){return 4*(int)(b-a);}
This is because (int)(b-a) could be a truncation in which case
multiplying with 4 might not result in the same value as
b-a truncated(?). The comment before the unpatched patterns
said "sign-changing conversions" but nothign actually verified this.
Might be that truncations are indeed ok now that I think about it.
Btw, do you have a better suggestion as to how to handle the original
issue rather than not relying on operand_equal_p for constants?
Richard.
Re: [PATCH] Fix PR77826
On Fri, 7 Oct 2016, Richard Biener wrote:
On Thu, 6 Oct 2016, Marc Glisse wrote:
On Wed, 5 Oct 2016, Richard Biener wrote:
The following will fix PR77826, the issue that in match.pd matching
up two things uses operand_equal_p which is too lax about the type
of the toplevel entity (at least for integer constants).
Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
The following is what I have applied.
Richard.
2016-10-05 Richard Biener
PR middle-end/77826
* genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
with types_match for GIMPLE code gen to handle type mismatched
constants properly.
I don't understand the disparity between generic and gimple here. Why let
(char)1 and (long)1 match in generic but not in gimple? And there are probably
several transformations in match.pd that could do with an update if constants
don't match anymore. Or did I misunderstand what the patch does?
The disparity is mostly that with GENERIC unfolded trees such as (char)1
are a bug while in GIMPLE the fact that the match.pd machinery does
valueization makes those a "feature" we have to deal with. Originally
I've restricted GENERIC as well but with its types_match_p implementation
it resulted in too many missed matches.
I shouldn't have written (long)1, I meant the fact that 1 (as a char
constant) and 1 (as a long constant) will now be matching captures in
generic and not in gimple. If we are going in the direction of not
matching constants of different types, I'd rather do it consistently and
update the patterns as needed to avoid the missed optimizations. The
missed matches exist in gimple as well, and generic optimization seems
less important than gimple to me.
An example that regressed at -O (looking at the .optimized dump)
int f(int a, unsigned b){
a &= 1;
b &= 1;
return a&b;
}
If we stick to the old behavior, maybe we could have some genmatch magic
to help with the constant capture weirdness. With matching captures, we
could select which operand (among those supposed to be equivalent) is
actually captured more cleverly, either with an explicit marker, or by
giving priority to the one that is not immediatly below convert? in the
pattern.
And if we move to stricter matching, maybe genmatch could be updated so
convert could also match integer constants in some cases.
I agree that some transforms would need updates - I've actually tried
to implement a warning for genmatch whenever seeing a match with
(T)@0 but there isn't any good existing place to sneak that in.
* match.pd ((X /[ex] A) * A -> X): Properly handle converted
and constant A.
This regressed
int f(int*a,int*b){return 4*(int)(b-a);}
This is because (int)(b-a) could be a truncation in which case
multiplying with 4 might not result in the same value as
b-a truncated(?). The comment before the unpatched patterns
said "sign-changing conversions" but nothign actually verified this.
Might be that truncations are indeed ok now that I think about it.
2015-05-22 Marc Glisse
PR tree-optimization/63387
* match.pd ((X /[ex] A) * A -> X): Remove unnecessary condition.
Apparently I forgot to remove the comment at that time :-(
Btw, do you have a better suggestion as to how to handle the original
issue rather than not relying on operand_equal_p for constants?
In previous cases, in order to get the right version of a matching
capture, we used non-matching captures and an explicit call to
operand_equal_p, for instance:
/* X - (X / Y) * Y is the same as X % Y. */
(simplify
(minus (convert1? @2) (convert2? (mult:c (trunc_div @0 @1) @1)))
/* We cannot use matching captures here, since in the case of
constants we really want the type of @0, not @2. */
(if (operand_equal_p (@0, @2, 0)
&& (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type)))
(convert (trunc_mod @0 @1
That seems to match the solutions that Jakub and you were discussing in
the PR, something like it should work (we can discuss the exact code).
I don't know if it is better. The old behavior of matching captures with
inconsistent types was confusing. A behavior with strict matching may
complicate things (will we duplicate patterns, or write operand_equal_p
explicitly to mimic the old behavior?). The recent inconsistency between
generic and gimple doesnt appeal to me much...
--
Marc Glisse
Re: [PATCH] Fix PR77826
On Mon, 10 Oct 2016, Marc Glisse wrote:
> On Fri, 7 Oct 2016, Richard Biener wrote:
>
> > On Thu, 6 Oct 2016, Marc Glisse wrote:
> >
> > > On Wed, 5 Oct 2016, Richard Biener wrote:
> > >
> > > > > The following will fix PR77826, the issue that in match.pd matching
> > > > > up two things uses operand_equal_p which is too lax about the type
> > > > > of the toplevel entity (at least for integer constants).
> > > > >
> > > > > Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
> > > >
> > > > The following is what I have applied.
> > > >
> > > > Richard.
> > > >
> > > > 2016-10-05 Richard Biener
> > > >
> > > > PR middle-end/77826
> > > > * genmatch.c (dt_operand::gen_match_op): Amend operand_equal_p
> > > > with types_match for GIMPLE code gen to handle type mismatched
> > > > constants properly.
> > >
> > > I don't understand the disparity between generic and gimple here. Why let
> > > (char)1 and (long)1 match in generic but not in gimple? And there are
> > > probably
> > > several transformations in match.pd that could do with an update if
> > > constants
> > > don't match anymore. Or did I misunderstand what the patch does?
> >
> > The disparity is mostly that with GENERIC unfolded trees such as (char)1
> > are a bug while in GIMPLE the fact that the match.pd machinery does
> > valueization makes those a "feature" we have to deal with. Originally
> > I've restricted GENERIC as well but with its types_match_p implementation
> > it resulted in too many missed matches.
>
> I shouldn't have written (long)1, I meant the fact that 1 (as a char constant)
> and 1 (as a long constant) will now be matching captures in generic and not in
> gimple. If we are going in the direction of not matching constants of
> different types, I'd rather do it consistently and update the patterns as
> needed to avoid the missed optimizations. The missed matches exist in gimple
> as well, and generic optimization seems less important than gimple to me.
Yes, I agree. I'll see on looking at the GENERIC fallout in more detail
(fixing patterns).
> An example that regressed at -O (looking at the .optimized dump)
>
> int f(int a, unsigned b){
> a &= 1;
> b &= 1;
> return a&b;
> }
Yeah, but it usually also shows a badly written pattern:
/* (X & Y) & (X & Z) -> (X & Y) & Z
(X | Y) | (X | Z) -> (X | Y) | Z */
(for op (bit_and bit_ior)
(simplify
(op:c (convert1?@3 (op:c@4 @0 @1)) (convert2?@5 (op:c@6 @0 @2)))
(if (tree_nop_conversion_p (type, TREE_TYPE (@1))
&& tree_nop_conversion_p (type, TREE_TYPE (@2)))
so how could we ever match the @0s when we have either of the two
conversions not present? We could only do this then facing constants
(due to using operand_equal_p). We for example fail to handle
(X & Y) & (unsigned) ((singed)X & Z)
> If we stick to the old behavior, maybe we could have some genmatch magic to
> help with the constant capture weirdness. With matching captures, we could
> select which operand (among those supposed to be equivalent) is actually
> captured more cleverly, either with an explicit marker, or by giving priority
> to the one that is not immediatly below convert? in the pattern.
This route is a difficult one to take -- what would be possible is to
capture a specific operand. Like allow one to write
(op (op @0@4 @1) (op @0@3 @2))
and thus actually have three "names" for @0. We have this internally
already when you write
(convert?@0 @1)
for the case where there is no conversion. @0 and @1 are the same
in this case.
Not sure if this helps enough cases.
I still think that having two things matched that are not really
the same is werid (and a possible source of errors as in, ICEs,
rather than missed optimizations).
> And if we move to stricter matching, maybe genmatch could be updated so
> convert could also match integer constants in some cases.
You mean when trying to match @0 ... (convert @0) and @0 is an INTEGER_CST
allow then (convert ...) case to match an INTEGER_CST of different type?
That's an interesting idea (not sure how to implement that though).
> > I agree that some transforms would need updates - I've actually tried
> > to implement a warning for genmatch whenever seeing a match with
> > (T)@0 but there isn't any good existing place to sneak that in.
>
>
> > > > * match.pd ((X /[ex] A) * A -> X): Properly handle converted
> > > > and constant A.
> > >
> > > This regressed
> > > int f(int*a,int*b){return 4*(int)(b-a);}
> >
> > This is because (int)(b-a) could be a truncation in which case
> > multiplying with 4 might not result in the same value as
> > b-a truncated(?). The comment before the unpatched patterns
> > said "sign-changing conversions" but nothign actually verified this.
> > Might be that truncations are indeed ok now that I think about it.
>
> 2015-05-22 Marc Glisse
>
> PR tree-optimization/63387
> * match.pd ((X /[ex] A) * A -> X): Remove unnecessar
Re: [PATCH] Fix PR77826
On Tue, 11 Oct 2016, Richard Biener wrote:
An example that regressed at -O (looking at the .optimized dump)
int f(int a, unsigned b){
a &= 1;
b &= 1;
return a&b;
}
Yeah, but it usually also shows a badly written pattern:
/* (X & Y) & (X & Z) -> (X & Y) & Z
(X | Y) | (X | Z) -> (X | Y) | Z */
(for op (bit_and bit_ior)
(simplify
(op:c (convert1?@3 (op:c@4 @0 @1)) (convert2?@5 (op:c@6 @0 @2)))
(if (tree_nop_conversion_p (type, TREE_TYPE (@1))
&& tree_nop_conversion_p (type, TREE_TYPE (@2)))
so how could we ever match the @0s when we have either of the two
conversions not present? We could only do this then facing constants
(due to using operand_equal_p). We for example fail to handle
(X & Y) & (unsigned) ((singed)X & Z)
Indeed... (oups, looks like I wrote that one)
Trying to find other examples
/* Fold A - (A & B) into ~B & A. */
(simplify
(minus (convert? @0) (convert?:s (bit_and:cs @0 @1)))
(if (tree_nop_conversion_p (type, TREE_TYPE (@0))
&& tree_nop_conversion_p (type, TREE_TYPE (@1)))
(convert (bit_and (bit_not @1) @0
Hmm, should be convert1/convert2 to handle the case where @0 is a
constant.
/* (X | Y) ^ X -> Y & ~ X*/
(simplify
(bit_xor:c (convert? (bit_ior:c @0 @1)) (convert? @0))
(if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
(convert (bit_and @1 (bit_not @0)
Again, will never match when there is a convert and @0 is a constant.
(for op (bit_and bit_ior bit_xor)
rop (bit_ior bit_and bit_and)
(simplify
(op (convert? (rop:c @0 @1)) (convert? (rop:c @0 @2)))
...
Again won't match for constant @0 that has a different type in both parts.
/* (X & Y) & Y -> X & Y
(X | Y) | Y -> X | Y */
(for op (bit_and bit_ior)
(simplify
(op:c (convert?@2 (op:c @0 @1)) (convert? @1))
@2))
Same issue.
Ok, not many transformations are concerned, and most need a rewrite
anyway...
In the other direction, looking at the transformations for which we used
explicitly operand_equal_p as a workaround for lax integer constant
matches, it doesn't look like changing them back to use matching captures
would help, it would require duplicating the pattern for constants.
If we stick to the old behavior, maybe we could have some genmatch magic to
help with the constant capture weirdness. With matching captures, we could
select which operand (among those supposed to be equivalent) is actually
captured more cleverly, either with an explicit marker, or by giving priority
to the one that is not immediatly below convert? in the pattern.
This route is a difficult one to take
The simplest version I was thinking about was @0 for a true capture, and
@@0 for something that just has to be checked for equality with @0.
-- what would be possible is to
capture a specific operand. Like allow one to write
(op (op @0@4 @1) (op @0@3 @2))
and thus actually have three "names" for @0. We have this internally
already when you write
(convert?@0 @1)
for the case where there is no conversion. @0 and @1 are the same
in this case.
Looks nice and convenient (assuming lax constant matching).
Not sure if this helps enough cases.
IIRC, in all cases where we had trouble with operand_equal_p, chosing
which operand to capture would have solved the issue.
I still think that having two things matched that are not really
the same is werid (and a possible source of errors as in, ICEs,
rather than missed optimizations).
Ok. Let's go with the strict matching, it is indeed less confusing.
And if we move to stricter matching, maybe genmatch could be updated so
convert could also match integer constants in some cases.
You mean when trying to match @0 ... (convert @0) and @0 is an INTEGER_CST
allow then (convert ...) case to match an INTEGER_CST of different type?
That's an interesting idea (not sure how to implement that though).
Yes, though I am not sure of the exact semantics that would work best.
On a related note, seeing duplicated patterns for constants, I tried
several variants of
(match (mynot @0)
(bit_not @0))
(match (mynot @0)
INTEGER_CST@0
(if (@0 = wide_int_to_tree (TREE_TYPE (@0), wi::bit_not (@0)
(simplify
(minus (bit_and:cs @0 (mynot @1)) (bit_and:cs @0 @1))
(minus (bit_xor @0 @1) @1))
This kind of hack feels wrong, but I don't see a proper way to write it.
I agree that some transforms would need updates - I've actually tried
to implement a warning for genmatch whenever seeing a match with
(T)@0 but there isn't any good existing place to sneak that in.
* match.pd ((X /[ex] A) * A -> X): Properly handle converted
and constant A.
This regressed
int f(int*a,int*b){return 4*(int)(b-a);}
This is because (int)(b-a) could be a truncation in which case
multiplying with 4 might not result in the same value as
b-a truncated(?). The comment before the unpatched patterns
said "sign-changing conversions" but nothign actually verified this.
Might be that truncations are indeed ok now that I think about it.
2015-0
Re: [PATCH] Fix PR77826
On Tue, 11 Oct 2016, Marc Glisse wrote:
> On Tue, 11 Oct 2016, Richard Biener wrote:
>
> > > An example that regressed at -O (looking at the .optimized dump)
> > >
> > > int f(int a, unsigned b){
> > > a &= 1;
> > > b &= 1;
> > > return a&b;
> > > }
> >
> > Yeah, but it usually also shows a badly written pattern:
> >
> > /* (X & Y) & (X & Z) -> (X & Y) & Z
> > (X | Y) | (X | Z) -> (X | Y) | Z */
> > (for op (bit_and bit_ior)
> > (simplify
> > (op:c (convert1?@3 (op:c@4 @0 @1)) (convert2?@5 (op:c@6 @0 @2)))
> > (if (tree_nop_conversion_p (type, TREE_TYPE (@1))
> > && tree_nop_conversion_p (type, TREE_TYPE (@2)))
> >
> > so how could we ever match the @0s when we have either of the two
> > conversions not present? We could only do this then facing constants
> > (due to using operand_equal_p). We for example fail to handle
> >
> > (X & Y) & (unsigned) ((singed)X & Z)
>
> Indeed... (oups, looks like I wrote that one)
>
> Trying to find other examples
>
> /* Fold A - (A & B) into ~B & A. */
> (simplify
> (minus (convert? @0) (convert?:s (bit_and:cs @0 @1)))
> (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
> && tree_nop_conversion_p (type, TREE_TYPE (@1)))
> (convert (bit_and (bit_not @1) @0
>
> Hmm, should be convert1/convert2 to handle the case where @0 is a constant.
>
> /* (X | Y) ^ X -> Y & ~ X*/
> (simplify
> (bit_xor:c (convert? (bit_ior:c @0 @1)) (convert? @0))
> (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
> (convert (bit_and @1 (bit_not @0)
>
> Again, will never match when there is a convert and @0 is a constant.
>
> (for op (bit_and bit_ior bit_xor)
> rop (bit_ior bit_and bit_and)
> (simplify
> (op (convert? (rop:c @0 @1)) (convert? (rop:c @0 @2)))
> ...
>
> Again won't match for constant @0 that has a different type in both parts.
>
> /* (X & Y) & Y -> X & Y
>(X | Y) | Y -> X | Y */
> (for op (bit_and bit_ior)
> (simplify
> (op:c (convert?@2 (op:c @0 @1)) (convert? @1))
> @2))
>
> Same issue.
>
> Ok, not many transformations are concerned, and most need a rewrite anyway...
>
> In the other direction, looking at the transformations for which we used
> explicitly operand_equal_p as a workaround for lax integer constant matches,
> it doesn't look like changing them back to use matching captures would help,
> it would require duplicating the pattern for constants.
So with doing the same on GENERIC I hit
FAIL: g++.dg/cpp1y/constexpr-array4.C -std=c++14 (test for excess errors)
with the pattern
/* (T)(P + A) - (T)P -> (T) A */
(for add (plus pointer_plus)
(simplify
(minus (convert (add @0 @1))
(convert @0))
...
(convert @1
no longer applying to
(long int) ((int *) &ar + 12) - (long int) &ar
because while (int *) &ar is equal to (long int) &ar (operand_equal_p
does STRIP_NOPS) they obviously do not have the same type. So on
GENERIC we have to consider that we feed operand_equal_p with
non-ATOMs (arbitrary expressions). The pattern above is "safe" as
it doesn't reference @0 in the result (not sure if we should take
advantage of that in genmatch).
The other FAILs with doing the same on GENERIC are
FAIL: g++.dg/gomp/declare-simd-3.C -std=gnu++11 (test for excess errors)
FAIL: g++.dg/torture/pr71448.C -O0 (test for excess errors)
FAIL: g++.dg/vect/simd-clone-6.cc -std=c++11 (test for excess errors)
the simd ones are 'warning: ignoring large linear step' and the pr71448.C
case is very similar to the above.
> > > If we stick to the old behavior, maybe we could have some genmatch magic
> > > to
> > > help with the constant capture weirdness. With matching captures, we could
> > > select which operand (among those supposed to be equivalent) is actually
> > > captured more cleverly, either with an explicit marker, or by giving
> > > priority
> > > to the one that is not immediatly below convert? in the pattern.
> >
> > This route is a difficult one to take
>
> The simplest version I was thinking about was @0 for a true capture, and @@0
> for something that just has to be checked for equality with @0.
Hmm, ok. So you'd have @@0 having to match @0 and we'd get the @0 for
the result in a reliable way? Sounds like a reasonable idea, I'll see
how that works out (we could auto-detect '@@' if the capture is not
used in the result, see above).
> > -- what would be possible is to
> > capture a specific operand. Like allow one to write
> >
> > (op (op @0@4 @1) (op @0@3 @2))
> >
> > and thus actually have three "names" for @0. We have this internally
> > already when you write
> >
> > (convert?@0 @1)
> >
> > for the case where there is no conversion. @0 and @1 are the same
> > in this case.
>
> Looks nice and convenient (assuming lax constant matching).
Yes, w/o lax matching it has of course little value.
> > Not sure if this helps enough cases.
>
> IIRC, in all cases where we had trouble with operand_equal_p, chosing which
> operand to capture would have solved the issue.
Y
Re: [PATCH] Fix PR77826
On Wed, 12 Oct 2016, Richard Biener wrote: So with doing the same on GENERIC I hit FAIL: g++.dg/cpp1y/constexpr-array4.C -std=c++14 (test for excess errors) with the pattern /* (T)(P + A) - (T)P -> (T) A */ (for add (plus pointer_plus) (simplify (minus (convert (add @0 @1)) (convert @0)) Ah, grep missed that one because it is on 2 lines :-( ... (convert @1 no longer applying to (long int) ((int *) &ar + 12) - (long int) &ar because while (int *) &ar is equal to (long int) &ar (operand_equal_p does STRIP_NOPS) they obviously do not have the same type. I believe we are comparing (int *) &ar to &ar, not to (long int) &ar. But yes, indeed. So on GENERIC we have to consider that we feed operand_equal_p with non-ATOMs (arbitrary expressions). The pattern above is "safe" as it doesn't reference @0 in the result (not sure if we should take advantage of that in genmatch). Since we are in the process of defining an operand_equal_for_(generic|gimple)_match_p, we could tweak it to check the type only for INTEGER_CST, or to still STRIP_NOPS, or similar. Or we could remain very strict and refine the pattern, allowing a convert on the pointer (we might want to split the plus and pointer_plus versions then, for clarity). There are not many optimizations that are mandated by front-ends and for which this is an issue. The other FAILs with doing the same on GENERIC are FAIL: g++.dg/gomp/declare-simd-3.C -std=gnu++11 (test for excess errors) FAIL: g++.dg/torture/pr71448.C -O0 (test for excess errors) FAIL: g++.dg/vect/simd-clone-6.cc -std=c++11 (test for excess errors) the simd ones are 'warning: ignoring large linear step' and the pr71448.C case is very similar to the above. Yes, I expect they all come from the same 1 or 2 transformations. If we stick to the old behavior, maybe we could have some genmatch magic to help with the constant capture weirdness. With matching captures, we could select which operand (among those supposed to be equivalent) is actually captured more cleverly, either with an explicit marker, or by giving priority to the one that is not immediatly below convert? in the pattern. This route is a difficult one to take The simplest version I was thinking about was @0 for a true capture, and @@0 for something that just has to be checked for equality with @0. Hmm, ok. So you'd have @@0 having to match @0 and we'd get the @0 for the result in a reliable way? Sounds like a reasonable idea, I'll see how that works out (we could auto-detect '@@' if the capture is not used in the result, see above). It probably doesn't bring much compared to the @0@4 syntax you were suggesting below, so if that is easier... -- what would be possible is to capture a specific operand. Like allow one to write (op (op @0@4 @1) (op @0@3 @2)) and thus actually have three "names" for @0. We have this internally already when you write (convert?@0 @1) for the case where there is no conversion. @0 and @1 are the same in this case. Looks nice and convenient (assuming lax constant matching). Yes, w/o lax matching it has of course little value. Not sure if this helps enough cases. IIRC, in all cases where we had trouble with operand_equal_p, chosing which operand to capture would have solved the issue. Yes. We'd still need to actually catch all those cases... Being forced to chose which operand we capture (say with @ vs @@, making 2 occurences of @0 a syntax error) might help, but it would also require updating many patterns for which this was never an issue (easy but boring). I still think that having two things matched that are not really the same is werid (and a possible source of errors as in, ICEs, rather than missed optimizations). Ok. Let's go with the strict matching, it is indeed less confusing. I'll hold off with the GENERIC strict matching and will investigate the @@ idea (plus auto-detecting it). And if we move to stricter matching, maybe genmatch could be updated so convert could also match integer constants in some cases. You mean when trying to match @0 ... (convert @0) and @0 is an INTEGER_CST allow then (convert ...) case to match an INTEGER_CST of different type? That's an interesting idea (not sure how to implement that though). Yes, though I am not sure of the exact semantics that would work best. On a related note, seeing duplicated patterns for constants, I tried several variants of (match (mynot @0) (bit_not @0)) (match (mynot @0) INTEGER_CST@0 (if (@0 = wide_int_to_tree (TREE_TYPE (@0), wi::bit_not (@0) (simplify (minus (bit_and:cs @0 (mynot @1)) (bit_and:cs @0 @1)) (minus (bit_xor @0 @1) @1)) This kind of hack feels wrong, but I don't see a proper way to write it. Yes. The above is very much a hack. Must have been me inventing it just to avoid duplicating the pattern. Uh, no, don't worry, we don't have that hack in match.pd, we have 2 patterns. The hack is just me trying to rem
Re: [PATCH] Fix PR77826
On Wed, 12 Oct 2016, Marc Glisse wrote: > On Wed, 12 Oct 2016, Richard Biener wrote: > > > So with doing the same on GENERIC I hit > > > > FAIL: g++.dg/cpp1y/constexpr-array4.C -std=c++14 (test for excess errors) > > > > with the pattern > > > > /* (T)(P + A) - (T)P -> (T) A */ > > (for add (plus pointer_plus) > > (simplify > >(minus (convert (add @0 @1)) > > (convert @0)) > > Ah, grep missed that one because it is on 2 lines :-( > > >... > > (convert @1 > > > > > > no longer applying to > > > > (long int) ((int *) &ar + 12) - (long int) &ar > > > > because while (int *) &ar is equal to (long int) &ar (operand_equal_p > > does STRIP_NOPS) they obviously do not have the same type. > > I believe we are comparing (int *) &ar to &ar, not to (long int) &ar. But yes, > indeed. > > > So on GENERIC we have to consider that we feed operand_equal_p with > > non-ATOMs (arbitrary expressions). The pattern above is "safe" as it > > doesn't reference @0 in the result (not sure if we should take advantage of > > that in genmatch). > > Since we are in the process of defining an > operand_equal_for_(generic|gimple)_match_p, we could tweak it to check the > type only for INTEGER_CST, or to still STRIP_NOPS, or similar. > > Or we could remain very strict and refine the pattern, allowing a convert on > the pointer (we might want to split the plus and pointer_plus versions then, > for clarity). There are not many optimizations that are mandated by front-ends > and for which this is an issue. > > > The other FAILs with doing the same on GENERIC are > > > > FAIL: g++.dg/gomp/declare-simd-3.C -std=gnu++11 (test for excess errors) > > FAIL: g++.dg/torture/pr71448.C -O0 (test for excess errors) > > FAIL: g++.dg/vect/simd-clone-6.cc -std=c++11 (test for excess errors) > > > > the simd ones are 'warning: ignoring large linear step' and the pr71448.C > > case is very similar to the above. > > Yes, I expect they all come from the same 1 or 2 transformations. > > > > > > If we stick to the old behavior, maybe we could have some genmatch > > > > > magic to help with the constant capture weirdness. With matching > > > > > captures, we could select which operand (among those supposed to be > > > > > equivalent) is actually captured more cleverly, either with an > > > > > explicit marker, or by giving priority to the one that is not > > > > > immediatly below convert? in the pattern. > > > > > > > > This route is a difficult one to take > > > > > > The simplest version I was thinking about was @0 for a true capture, and > > > @@0 > > > for something that just has to be checked for equality with @0. > > > > Hmm, ok. So you'd have @@0 having to match @0 and we'd get the @0 for > > the result in a reliable way? Sounds like a reasonable idea, I'll see > > how that works out (we could auto-detect '@@' if the capture is not > > used in the result, see above). > > It probably doesn't bring much compared to the @0@4 syntax you were > suggesting below, so if that is easier... Will figure that out ... > > > > -- what would be possible is to > > > > capture a specific operand. Like allow one to write > > > > > > > > (op (op @0@4 @1) (op @0@3 @2)) > > > > > > > > and thus actually have three "names" for @0. We have this internally > > > > already when you write > > > > > > > > (convert?@0 @1) > > > > > > > > for the case where there is no conversion. @0 and @1 are the same > > > > in this case. > > > > > > Looks nice and convenient (assuming lax constant matching). > > > > Yes, w/o lax matching it has of course little value. > > > > > > Not sure if this helps enough cases. > > > > > > IIRC, in all cases where we had trouble with operand_equal_p, chosing > > > which > > > operand to capture would have solved the issue. > > > > Yes. We'd still need to actually catch all those cases... > > Being forced to chose which operand we capture (say with @ vs @@, making 2 > occurences of @0 a syntax error) might help, but it would also require > updating many patterns for which this was never an issue (easy but boring). We can even have today (plus (minus @0 @1) (plus @0 @2) @0) thus three matching @0. Now if we have @@ telling to use value equality rather than "node equality" _and_ making the @@ select the canonical operand to choose then we'd have at most one @@ per ID. Somewhat tricky but not impossible to implement I think. > > > > I still think that having two things matched that are not really > > > > the same is werid (and a possible source of errors as in, ICEs, > > > > rather than missed optimizations). > > > > > > Ok. Let's go with the strict matching, it is indeed less confusing. > > > > I'll hold off with the GENERIC strict matching and will investigate > > the @@ idea (plus auto-detecting it). > > > > > > > And if we move to stricter matching, maybe genmatch could be updated > > > > > so > > > > > convert could also match integer constants in some cases. > > > > > > >
Re: [PATCH] Fix PR77826
On Wed, 12 Oct 2016, Richard Biener wrote: > On Wed, 12 Oct 2016, Marc Glisse wrote: > > > On Wed, 12 Oct 2016, Richard Biener wrote: > > > > > So with doing the same on GENERIC I hit > > > > > > FAIL: g++.dg/cpp1y/constexpr-array4.C -std=c++14 (test for excess errors) > > > > > > with the pattern > > > > > > /* (T)(P + A) - (T)P -> (T) A */ > > > (for add (plus pointer_plus) > > > (simplify > > >(minus (convert (add @0 @1)) > > > (convert @0)) > > > > Ah, grep missed that one because it is on 2 lines :-( > > > > >... > > > (convert @1 > > > > > > > > > no longer applying to > > > > > > (long int) ((int *) &ar + 12) - (long int) &ar > > > > > > because while (int *) &ar is equal to (long int) &ar (operand_equal_p > > > does STRIP_NOPS) they obviously do not have the same type. > > > > I believe we are comparing (int *) &ar to &ar, not to (long int) &ar. But > > yes, > > indeed. > > > > > So on GENERIC we have to consider that we feed operand_equal_p with > > > non-ATOMs (arbitrary expressions). The pattern above is "safe" as it > > > doesn't reference @0 in the result (not sure if we should take advantage > > > of > > > that in genmatch). > > > > Since we are in the process of defining an > > operand_equal_for_(generic|gimple)_match_p, we could tweak it to check the > > type only for INTEGER_CST, or to still STRIP_NOPS, or similar. > > > > Or we could remain very strict and refine the pattern, allowing a convert on > > the pointer (we might want to split the plus and pointer_plus versions then, > > for clarity). There are not many optimizations that are mandated by > > front-ends > > and for which this is an issue. > > > > > The other FAILs with doing the same on GENERIC are > > > > > > FAIL: g++.dg/gomp/declare-simd-3.C -std=gnu++11 (test for excess errors) > > > FAIL: g++.dg/torture/pr71448.C -O0 (test for excess errors) > > > FAIL: g++.dg/vect/simd-clone-6.cc -std=c++11 (test for excess errors) > > > > > > the simd ones are 'warning: ignoring large linear step' and the pr71448.C > > > case is very similar to the above. > > > > Yes, I expect they all come from the same 1 or 2 transformations. > > > > > > > > If we stick to the old behavior, maybe we could have some genmatch > > > > > > magic to help with the constant capture weirdness. With matching > > > > > > captures, we could select which operand (among those supposed to be > > > > > > equivalent) is actually captured more cleverly, either with an > > > > > > explicit marker, or by giving priority to the one that is not > > > > > > immediatly below convert? in the pattern. > > > > > > > > > > This route is a difficult one to take > > > > > > > > The simplest version I was thinking about was @0 for a true capture, and > > > > @@0 > > > > for something that just has to be checked for equality with @0. > > > > > > Hmm, ok. So you'd have @@0 having to match @0 and we'd get the @0 for > > > the result in a reliable way? Sounds like a reasonable idea, I'll see > > > how that works out (we could auto-detect '@@' if the capture is not > > > used in the result, see above). > > > > It probably doesn't bring much compared to the @0@4 syntax you were > > suggesting below, so if that is easier... > > Will figure that out ... > > > > > > -- what would be possible is to > > > > > capture a specific operand. Like allow one to write > > > > > > > > > > (op (op @0@4 @1) (op @0@3 @2)) > > > > > > > > > > and thus actually have three "names" for @0. We have this internally > > > > > already when you write > > > > > > > > > > (convert?@0 @1) > > > > > > > > > > for the case where there is no conversion. @0 and @1 are the same > > > > > in this case. > > > > > > > > Looks nice and convenient (assuming lax constant matching). > > > > > > Yes, w/o lax matching it has of course little value. > > > > > > > > Not sure if this helps enough cases. > > > > > > > > IIRC, in all cases where we had trouble with operand_equal_p, chosing > > > > which > > > > operand to capture would have solved the issue. > > > > > > Yes. We'd still need to actually catch all those cases... > > > > Being forced to chose which operand we capture (say with @ vs @@, making 2 > > occurences of @0 a syntax error) might help, but it would also require > > updating many patterns for which this was never an issue (easy but boring). > > We can even have today > > (plus (minus @0 @1) (plus @0 @2) @0) > > thus three matching @0. Now if we have @@ telling to use value equality > rather than "node equality" _and_ making the @@ select the canonical > operand to choose then we'd have at most one @@ per ID. Somewhat tricky > but not impossible to implement I think. So here it is. It allows us to remove the operand_equal_p hacks: @@ -334,11 +334,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) /* X - (X / Y) * Y is the same as X % Y. */ (simplify - (minus (convert1? @2) (convert2? (mult:c (trunc_div @0 @1) @1))) - /* We cannot use mat
Re: [PATCH] Fix PR77826
On Thu, 13 Oct 2016, Richard Biener wrote: The patch introduces '@@' captures which do two things - first they change the comparison used for matching back to operand_equal_p only (thus perform "value-matching"), second the @@ capture denotes the specific operand that should be refered to in the result section (ifs and result expressions). When we face (plus @0 @1) (convert? @@0) for example this is lowered to (plus @__2 @1) (convert? @__2@0) marking the @__2 match for value handling. Funny, I had the opposite convention in mind ;-) On a completely artificial example: (minus (plus (plus @0 @@0) @@0) @0) would correspond to: (minus (plus (plus @1 @2) @3) @4) where either @1 or @4 is captured as @0, say @1 for example, @4 is compared strictly to @1, while @2 and @3 are value-compared to @1 (lax). This way, when we talk about @0 later in the transformation, we are indeed talking about the thing that was called @0 in the input pattern, while @@0 is not-quite-@0. But your version should be fine. I modified the patterns you identified and the ones I did and removed the operand_equal_p uses where possible. Thanks. (some can be further generalized, as you explained in an earlier message, but we can do that later as the need arises) -- Marc Glisse
Re: [PATCH] Fix PR77826
On Sat, 15 Oct 2016, Marc Glisse wrote: > On Thu, 13 Oct 2016, Richard Biener wrote: > > > The patch introduces '@@' captures which do two things - first they > > change the comparison used for matching back to operand_equal_p > > only (thus perform "value-matching"), second the @@ capture denotes > > the specific operand that should be refered to in the result > > section (ifs and result expressions). > > > > When we face (plus @0 @1) (convert? @@0) for example this is lowered > > to (plus @__2 @1) (convert? @__2@0) marking the @__2 match for > > value handling. > > Funny, I had the opposite convention in mind ;-) > > On a completely artificial example: > (minus (plus (plus @0 @@0) @@0) @0) > > would correspond to: > (minus (plus (plus @1 @2) @3) @4) > > where either @1 or @4 is captured as @0, say @1 for example, @4 is compared > strictly to @1, while @2 and @3 are value-compared to @1 (lax). > > This way, when we talk about @0 later in the transformation, we are indeed > talking about the thing that was called @0 in the input pattern, while @@0 is > not-quite-@0. > > But your version should be fine. Yeah, I tried to avoid any ambiguity if @@ is used and didn't like to write (minus (plus (plus @0 @@0) @@0) @@0) (too many @@s). > > I modified the patterns you identified and the ones I did and > > removed the operand_equal_p uses where possible. > > Thanks. (some can be further generalized, as you explained in an earlier > message, but we can do that later as the need arises) Yes. Richard.
