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 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)))
+ (minus (convert1? @0) (convert2? (mult:c (trunc_div @@0 @@1) @1)))
+ (if (INTEGRAL_TYPE_P (type) || VECTOR_INTEGER_TYPE_P (type))
   (convert (trunc_mod @0 @1))))

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.

I modified the patterns you identified and the ones I did and
removed the operand_equal_p uses where possible.

I did not implement the auto-guessing idea as I usually like
to be more explicit here.

Bootstrap and regtest running on x86_64-unknown-linux-gnu.

Richard.

2016-10-13  Richard Biener  <rguenther@suse.de>

	PR middle-end/77826
	* genmatch.c (struct capture): Add value_match member.
	(commutate): Preserve value_match.
	(lower_opt_convert): Likewise.
	(lower_cond): Likewise.
	(replace_id): Likewise.
	(struct dt_operand): Add value_match member.
	(decision_tree::cmp_node): Compare it.
	(decision_tree::insert_operand): Honor it when finding and
	when appending a DT_MATCH.
	(dt_operand::gen_match_op): Generate a type check after
	operand_equal_p if ! value_match for both GENERIC and GIMPLE.
	(parser::get_internal_capture_id): New helper.
	(parser::finish_match_operand): New function lowering @@<id>.
	(parser::parse_capture): Parse @@<id> as value-match.
	(parser::parse_expr): Use get_internal_capture_id.
	(parser::parse_simplify): Call finish_match_operand.
	(walk_captures): New helper.
	* match.pd (X - (X / Y) * Y -> X % Y): Use value-matching instead
	of operand_equal_p.
	((X /[ex] A) * A -> X): Likewise.
	((X | Y) ^ X -> Y & ~ X): Handle constants properly by using
	convert[12] and value-matching.
	((A | B) & (A | C) ->  A | (B & C)): Likewise.
	((X | Y) | Y -> X | Y): Likewise.
	((X ^ Y) ^ Y -> X): Likewise.
	(A - (A & B) -> ~B & A): Likewise.
	((T)(P + A) - (T)P -> (T) A): Likewise.
	((T)P - (T)(P + A) -> -(T) A): Likewise.
	((T)(P + A) - (T)(P + B) -> (T)A - (T)B): Likewise.
	* doc/match-and-simplify.texi: Amend capture section.


Fix PR77826

Commit Message

Comments

Patch