[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #9 from Richard Biener  ---
Author: rguenth
Date: Thu Oct 13 12:15:38 2016
New Revision: 241108

URL: https://gcc.gnu.org/viewcvs?rev=241108=gcc=rev
Log:
2016-10-13  Richard Biener  

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 @@.
(parser::parse_capture): Parse @@ 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.

Modified:
trunk/gcc/ChangeLog
trunk/gcc/doc/match-and-simplify.texi
trunk/gcc/genmatch.c
trunk/gcc/match.pd

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #8 from Richard Biener  ---
Author: rguenth
Date: Wed Oct  5 11:38:59 2016
New Revision: 240776

URL: https://gcc.gnu.org/viewcvs?rev=240776=gcc=rev
Log:
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.

Added:
trunk/gcc/testsuite/gcc.dg/torture/pr77826.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/genmatch.c
trunk/gcc/match.pd
trunk/gcc/testsuite/ChangeLog

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

Richard Biener  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #7 from Richard Biener  ---
Fixed.

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #6 from Richard Biener  ---
Fallout in GENERIC folding from doing that:

FAIL: c-c++-common/fold-divmul-1.c  -std=gnu++11  scan-tree-dump-not original
"/
[ex]"
FAIL: g++.dg/cpp1y/constexpr-array4.C  -std=c++14 (test for excess errors)
FAIL: g++.dg/ipa/devirt-28a.C  -std=gnu++11 (test for excess errors)
FAIL: g++.dg/gomp/declare-simd-3.C  -std=gnu++98  scan-assembler-times
_ZGVeN16v
ulLUR4__Z2f4iiiRKiS0_S0_: 1
FAIL: g++.dg/torture/pr71448.C   -O0  (test for excess errors)

FAIL: c-c++-common/fold-divmul-1.c  -Wc++-compat   scan-tree-dump-not original
"
/[ex]"
FAIL: gcc.dg/pr58742-1.c scan-tree-dump cddce1 "return e"

so for GENERIC the used types_match () is too strict it seems.

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #5 from rguenther at suse dot de  ---
On Tue, 4 Oct 2016, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826
> 
> --- Comment #4 from Jakub Jelinek  ---
> (In reply to Richard Biener from comment #3)
> > --- gcc/genmatch.c  (revision 240739)
> > +++ gcc/genmatch.c  (working copy)
> > @@ -2593,8 +2601,10 @@ dt_operand::gen_match_op (FILE *f, int i
> >  {
> >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);
> > +  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);
> >fprintf_indent (f, indent + 2, "{\n");
> >return 1;
> >  }
> 
> But that will mean the pattern will no longer match.

Yes.  I'm not sure if in all patterns a mismatched type wouldn't not
cause wrong-code eventually.

>  While it is generally
> safer, for the cases like this simplify, shall it then use (convert? @2)
> instead and add operand_equal_p for @0 and @2?  I guess all it cares is that 
> @0
> actually has the same type as @1 there, not a different one?

Well, they have the same type by construction (being operands to bit_ior).

The issue with constants is that if we had (int) x_2 and valueize x_2 to
1 then the matcher still sees (int) 1 thus valueized matching occurs on
an otherwise "unfolded" tree ...

The pattern fails to handle (long)(2 | Y) ^ 2L as well, so it really
would need to do

/* (X | Y) ^ X -> Y & ~ X*/
(simplify
 (bit_xor:c (convert1? (bit_ior:c @0 @1)) (convert2? @0))
 (if (tree_nop_conversion_p (type, TREE_TYPE (@0)))
  (convert (bit_and @1 (bit_not @0)

which then exposes the fact that if @0 == constant we might have
different types in the two places.  So it would need to be done as

/* (X | Y) ^ X -> Y & ~ X*/  
(simplify
 (bit_xor:c (convert1? (bit_ior:c @0 @1)) (convert2? @2))
 (if (tree_nop_conversion_p (type, TREE_TYPE (@0))
  && (@0 == @2 || operand_equal_p (@0, @2, 0)))
  (convert (bit_and @1 (bit_not @0)


Anyway - I think the safer operand_equal_p () && types_match () is
the way to go.  OTOH I don't like the explicit matching with
operand_equal_p ...

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #4 from Jakub Jelinek  ---
(In reply to Richard Biener from comment #3)
> --- gcc/genmatch.c  (revision 240739)
> +++ gcc/genmatch.c  (working copy)
> @@ -2593,8 +2601,10 @@ dt_operand::gen_match_op (FILE *f, int i
>  {
>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);
> +  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);
>fprintf_indent (f, indent + 2, "{\n");
>return 1;
>  }

But that will mean the pattern will no longer match.  While it is generally
safer, for the cases like this simplify, shall it then use (convert? @2)
instead and add operand_equal_p for @0 and @2?  I guess all it cares is that @0
actually has the same type as @1 there, not a different one?

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

--- Comment #3 from Richard Biener  ---
Ideally we'd do operand_equal_p's job in the pattern but there is no reliable
way to get at the type of the (convert? @0) operand.  So I'm thinking of
amending operand_equal_p () with a types_match () check instead...

Index: gcc/genmatch.c
===
--- gcc/genmatch.c  (revision 240739)
+++ gcc/genmatch.c  (working copy)
@@ -2593,8 +2601,10 @@ dt_operand::gen_match_op (FILE *f, int i
 {
   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);
+  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);
   fprintf_indent (f, indent + 2, "{\n");
   return 1;
 }

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

Richard Biener  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Assignee|unassigned at gcc dot gnu.org  |rguenth at gcc dot 
gnu.org
   Target Milestone|--- |7.0

--- Comment #2 from Richard Biener  ---
I believe we did run into this issue before.  I'll have a closer look into the
failure mode but as both conversions need to be there I can't see how it
matches.
As said, I'll have a look (but yes, the lazy type matching of operand_equal_p
is a problem...)

[Bug target/77826] [7 Regression] ICE in decompose, at wide-int.h:928 w/ -m64 -O2 and above

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77826

Jakub Jelinek  changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2016-10-03
 CC||jakub at gcc dot gnu.org
 Ever confirmed|0   |1

--- Comment #1 from Jakub Jelinek  ---
Started with r240291.
I think the problem is either in the
/* (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)
simplifier or in the infrastructure.
  _3 = *v0_14(D);
  _4 = (long unsigned int) q2_11(D);
  _5 = _3 | _4;
  *v0_14(D) = _5;
  _7 = (unsigned int) _5;
  _9 = q2.0_1 ^ _7;
The xor is done in unsigned int type, while ior is done in unsigned long int
type, and evrp is trying to valueize q2_11(D) as 1.  As it is valueized as 1
and operand_equal_p is true for 1U and 1UL, we seem to end up with @0 being 1U
(rather than 1UL), while @1 is unsigned long variable.
--- match.pd.jj 2016-09-29 22:53:14.0 +0200
+++ match.pd2016-10-03 16:04:50.905185018 +0200
@@ -707,8 +707,9 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 /* (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)
+ (if (tree_nop_conversion_p (type, TREE_TYPE (@1)))
+  (with { tree itype = TREE_TYPE (@1); }
+   (convert (bit_and @1 (bit_not (convert:itype @0)))

 /* Convert ~X ^ ~Y to X ^ Y.  */
 (simplify
fixes the ICE, but I'm not sure if it is the right fix.  Plus, there are
various other simplifications which use (convert? @[0-9]) together with the
same @[0-9] outside of the convert that could potentially have the same
problem.