Re: [PATCH] Fix PR77826

[Richard Biener <rguenther@suse.de>]

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.
> > > > 
> > > > 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 remove the duplication. If you thought you had
> written it, that means it may not have been such an absurd way to go about it
> ;-)

Ah ...

;)