diff mbox

[PR78507/PR78510] Fix two ICEs in pattern (cond (cmp (convert1? @1) @3) (convert2? @1) @2).

Message ID CAHFci29D=655SMbm9+QnN24de-8vKLZ2-wGgP9Ad60+1EwueSg@mail.gmail.com
State New
Headers show

Commit Message

Bin.Cheng Nov. 25, 2016, 11:20 a.m. UTC 
On Fri, Nov 25, 2016 at 8:23 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Nov 24, 2016 at 4:22 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>> Hi,
>> This patch fixes two issues in newly introduced pattern: (cond (cmp (convert1? @1) @3) (convert2? @1) @2).
>> For PR78507, we need to check if from_type is INTEGRAL_TYPE_P explicitly, this patch adds check for that.
>> Note we don't check c1_type/c2_type for that because it's guarded by INTEGER_CST@.
>> For PR78510, the ICE is covered by revision 242831, but underlying issue is when the block of conditions
>> is not satisfied, the last case simplification is applied anyway since code is initialized to cmp at the
>> first place.  This patch fixes that by setting code to proper value only when transformation is valid, it
>> also incorporates Marc's suggestion by using cmp directly.  Two tests are added too.
>> Bootstrap and test on x86_64 and AArch64 ongoing, is it OK if no failures?
>
> Ok.
>
> Btw,
>
> -  (cond (cmp@0 (convert1? @1) ...
> ...
> -     enum tree_code code = TREE_CODE (@0)...
>
> shows another misconception (I didn't notice...) in that TREE_CODE of a captured
> expression results in the expression code.  On GENERIC that works but on GIMPLE
> @0 will be a SSA_NAME and thus 'code' was SSA_NAME ...
Yes, that was a mistake.
Patch updated by including new test from duplicate PR78517.  Is it OK?

Thanks,
bin

2016-11-24  Bin Cheng  <bin.cheng@arm.com>

    PR middle-end/78507
    PR middle-end/78510
    PR middle-end/78517
    * match.pd ((cond (cmp (convert1? @1) @3) (convert2? @1) @2)): Use
    cmp directly, rather than cmp_code.  Initialize code to ERROR_MARK
    and set it to result code if transformation is valid.  Use code EQ
    directly in last simplification case.

gcc/testsuite/ChangeLog
2016-11-24  Bin Cheng  <bin.cheng@arm.com>

    PR middle-end/78507
    PR middle-end/78510
    PR middle-end/78517
    * g++.dg/torture/pr78507.C: New test.
    * gcc.dg/torture/pr78510.c: New test.
    * gcc.dg/torture/pr78517.c: New test.

>
> Thanks,
> Richard.
>
>> Thanks,
>> bin
>>
>> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>>
>>         PR middle-end/78507
>>         PR middle-end/78510
>>         * match.pd ((cond (cmp (convert1? @1) @3) (convert2? @1) @2)): Use
>>         cmp directly, rather than cmp_code.  Initialize code to ERROR_MARK
>>         and set it to result code if transformation is valid.  Use code EQ
>>         directly in last simplification case.
>>
>> gcc/testsuite/ChangeLog
>> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>>
>>         PR middle-end/78507
>>         PR middle-end/78510
>>         * g++.dg/torture/pr78507.C: New test.
>>         * gcc.dg/torture/pr78510.c: New test.

Comments

Richard Biener Nov. 25, 2016, 11:32 a.m. UTC | #1 
On Fri, Nov 25, 2016 at 12:20 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Fri, Nov 25, 2016 at 8:23 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Nov 24, 2016 at 4:22 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>> Hi,
>>> This patch fixes two issues in newly introduced pattern: (cond (cmp (convert1? @1) @3) (convert2? @1) @2).
>>> For PR78507, we need to check if from_type is INTEGRAL_TYPE_P explicitly, this patch adds check for that.
>>> Note we don't check c1_type/c2_type for that because it's guarded by INTEGER_CST@.
>>> For PR78510, the ICE is covered by revision 242831, but underlying issue is when the block of conditions
>>> is not satisfied, the last case simplification is applied anyway since code is initialized to cmp at the
>>> first place.  This patch fixes that by setting code to proper value only when transformation is valid, it
>>> also incorporates Marc's suggestion by using cmp directly.  Two tests are added too.
>>> Bootstrap and test on x86_64 and AArch64 ongoing, is it OK if no failures?
>>
>> Ok.
>>
>> Btw,
>>
>> -  (cond (cmp@0 (convert1? @1) ...
>> ...
>> -     enum tree_code code = TREE_CODE (@0)...
>>
>> shows another misconception (I didn't notice...) in that TREE_CODE of a captured
>> expression results in the expression code.  On GENERIC that works but on GIMPLE
>> @0 will be a SSA_NAME and thus 'code' was SSA_NAME ...
> Yes, that was a mistake.
> Patch updated by including new test from duplicate PR78517.  Is it OK?

Yes.

Richard.

> Thanks,
> bin
>
> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>
>     PR middle-end/78507
>     PR middle-end/78510
>     PR middle-end/78517
>     * match.pd ((cond (cmp (convert1? @1) @3) (convert2? @1) @2)): Use
>     cmp directly, rather than cmp_code.  Initialize code to ERROR_MARK
>     and set it to result code if transformation is valid.  Use code EQ
>     directly in last simplification case.
>
> gcc/testsuite/ChangeLog
> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>
>     PR middle-end/78507
>     PR middle-end/78510
>     PR middle-end/78517
>     * g++.dg/torture/pr78507.C: New test.
>     * gcc.dg/torture/pr78510.c: New test.
>     * gcc.dg/torture/pr78517.c: New test.
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> bin
>>>
>>> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR middle-end/78507
>>>         PR middle-end/78510
>>>         * match.pd ((cond (cmp (convert1? @1) @3) (convert2? @1) @2)): Use
>>>         cmp directly, rather than cmp_code.  Initialize code to ERROR_MARK
>>>         and set it to result code if transformation is valid.  Use code EQ
>>>         directly in last simplification case.
>>>
>>> gcc/testsuite/ChangeLog
>>> 2016-11-24  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR middle-end/78507
>>>         PR middle-end/78510
>>>         * g++.dg/torture/pr78507.C: New test.
>>>         * gcc.dg/torture/pr78510.c: New test.
diff mbox

Patch

diff --git a/gcc/match.pd b/gcc/match.pd
index 3aa27fa..2d4e019 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -1971,14 +1971,15 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
    (cond (eq (convert1? x) c1) (convert2? x) c2) -> (cond (eq x c1) c1 c2).  */
 (for cmp (lt le gt ge eq)
  (simplify
-  (cond (cmp@0 (convert1? @1) INTEGER_CST@3) (convert2? @1) INTEGER_CST@2)
+  (cond (cmp (convert1? @1) INTEGER_CST@3) (convert2? @1) INTEGER_CST@2)
   (with
    {
      tree from_type = TREE_TYPE (@1);
      tree c1_type = TREE_TYPE (@3), c2_type = TREE_TYPE (@2);
-     enum tree_code code = TREE_CODE (@0), cmp_code = TREE_CODE (@0);
+     enum tree_code code = ERROR_MARK;
 
-     if (int_fits_type_p (@2, from_type)
+     if (INTEGRAL_TYPE_P (from_type)
+	 && int_fits_type_p (@2, from_type)
 	 && (types_match (c1_type, from_type)
 	     || (TYPE_PRECISION (c1_type) > TYPE_PRECISION (from_type)
 		 && (TYPE_UNSIGNED (from_type)
@@ -1988,37 +1989,38 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
 		 && (TYPE_UNSIGNED (from_type)
 		     || TYPE_SIGN (c2_type) == TYPE_SIGN (from_type)))))
        {
-	 if (code != EQ_EXPR)
+	 if (cmp != EQ_EXPR)
 	   {
 	     if (wi::to_widest (@3) == (wi::to_widest (@2) - 1))
 	       {
 		 /* X <= Y - 1 equals to X < Y.  */
-		 if (cmp_code == LE_EXPR)
+		 if (cmp == LE_EXPR)
 		   code = LT_EXPR;
 		 /* X > Y - 1 equals to X >= Y.  */
-		 if (cmp_code == GT_EXPR)
+		 if (cmp == GT_EXPR)
 		   code = GE_EXPR;
 	       }
 	     if (wi::to_widest (@3) == (wi::to_widest (@2) + 1))
 	       {
 		 /* X < Y + 1 equals to X <= Y.  */
-		 if (cmp_code == LT_EXPR)
+		 if (cmp == LT_EXPR)
 		   code = LE_EXPR;
 		 /* X >= Y + 1 equals to X > Y.  */
-		 if (cmp_code == GE_EXPR)
+		 if (cmp == GE_EXPR)
 		   code = GT_EXPR;
 	       }
-	     if (code != cmp_code || wi::to_widest (@2) == wi::to_widest (@3))
+	     if (code != ERROR_MARK
+		 || wi::to_widest (@2) == wi::to_widest (@3))
 	       {
-		 if (cmp_code == LT_EXPR || cmp_code == LE_EXPR)
+		 if (cmp == LT_EXPR || cmp == LE_EXPR)
 		   code = MIN_EXPR;
-		 if (cmp_code == GT_EXPR || cmp_code == GE_EXPR)
+		 if (cmp == GT_EXPR || cmp == GE_EXPR)
 		   code = MAX_EXPR;
 	       }
 	   }
 	 /* Can do A == C1 ? A : C2  ->  A == C1 ? C1 : C2?  */
-	 else if (!int_fits_type_p (@3, from_type))
-	   code = ERROR_MARK;
+	 else if (int_fits_type_p (@3, from_type))
+	   code = EQ_EXPR;
        }
    }
    (if (code == MAX_EXPR)
@@ -2026,7 +2028,7 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     (if (code == MIN_EXPR)
      (convert (min @1 (convert @2)))
      (if (code == EQ_EXPR)
-      (convert (cond (cmp @1 (convert @3))
+      (convert (cond (eq @1 (convert @3))
 		     (convert:from_type @3) (convert:from_type @2)))))))))
 
 (for cnd (cond vec_cond)
diff --git a/gcc/testsuite/g++.dg/torture/pr78507.C b/gcc/testsuite/g++.dg/torture/pr78507.C
new file mode 100644
index 0000000..9691cf9
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr78507.C
@@ -0,0 +1,57 @@ 
+// PR middle-end/78507
+// { dg-do compile }
+struct A {
+  template <typename _Iterator1, typename _Iterator2>
+  int operator()(_Iterator1, _Iterator2);
+};
+struct B {
+  template <typename _BI1, typename _BI2>
+  static _BI2 __copy_move_b(_BI1 p1, _BI2 p2) {
+    _BI1 a;
+    long b = p1 - a;
+    for (; b > 0; --b)
+      *--p2 = *--p1;
+  }
+};
+template <int, typename _BI1, typename _BI2>
+void __copy_move_backward_a(_BI1 p1, _BI2 p2) {
+  B::__copy_move_b(p1, p2);
+}
+template <int, typename _BI1, typename _BI2>
+void __copy_move_backward_a2(_BI1 p1, _BI2 p2) {
+  __copy_move_backward_a<0>(p1, p2);
+}
+template <typename _BI1, typename _BI2> void move_backward(_BI1 p1, _BI2 p2) {
+  __copy_move_backward_a2<0>(p1, p2);
+}
+template <typename _RandomAccessIterator, typename _Compare>
+void __insertion_sort(_RandomAccessIterator, _Compare p2) {
+  for (_RandomAccessIterator c;; ++c)
+    if (p2(0, 0))
+      move_backward(c, c + 1);
+}
+template <typename _RandomAccessIterator, typename _Compare>
+void __final_insertion_sort(_RandomAccessIterator, _Compare p2) {
+  _RandomAccessIterator d;
+  __insertion_sort(d, p2);
+}
+template <typename _RandomAccessIterator, typename _Compare>
+void __sort(_RandomAccessIterator p1, _Compare p2) {
+  __final_insertion_sort(p1, p2);
+}
+template <typename _RandomAccessIterator, typename _Compare>
+void sort(_RandomAccessIterator, _RandomAccessIterator p2, _Compare) {
+  A e;
+  __sort(p2, e);
+}
+struct C {
+  struct D {
+    int DwarfRegNum;
+  };
+  int parseRegisterLiveOutMask() const;
+};
+int C::parseRegisterLiveOutMask() const {
+  D f, g;
+  sort(&f, &g, [] {});
+}
+
diff --git a/gcc/testsuite/gcc.dg/torture/pr78510.c b/gcc/testsuite/gcc.dg/torture/pr78510.c
new file mode 100644
index 0000000..8d0ab5f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr78510.c
@@ -0,0 +1,19 @@ 
+/* PR middle-end/78510 */
+/* { dg-do compile } */
+int a, b, c, e, f;
+char d;
+short g, h;
+char fn1(int p1) {
+  for (;;) {
+    h = p1 << 2;
+    int i = h;
+    g = i > 32767 >> 13 ? i : i << 3;
+    f = a ?: c;
+    if (e)
+      return d;
+  }
+}
+
+static int fn2() { fn1(0 || b); }
+
+int main() { fn2(); return 0; }
diff --git a/gcc/testsuite/gcc.dg/torture/pr78517.c b/gcc/testsuite/gcc.dg/torture/pr78517.c
new file mode 100644
index 0000000..475bce7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr78517.c
@@ -0,0 +1,4 @@ 
+/* PR middle-end/78517 */
+/* { dg-do compile } */
+char a;
+int fn1() { return a == '[' ? a : 0; }