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.
Yes. We'd still need to actually catch all those 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).
>
> 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.
Richard.
>
> > > > 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 <marc.gli...@inria.fr>
> > >
> > > PR tree-optimization/63387
> > > * match.pd ((X /[ex] A) * A -> X): Remove unnecessary condition.
> > >
> > > Apparently I forgot to remove the comment at that time :-(
> >
> > Ok. I'm now testing a patch to remove the restriction again (and adjust
> > the comment).
>
> Thanks. (since exact_div is used almost only with constant divisors, the old
> pattern was fine before strict matching, but indeed your more general pattern
> is better)
