[patch, ARM] ICE in get_arm_condition_code()

[patch, ARM] ICE in get_arm_condition_code()

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

Hi,
there's an ICE in arm.c:get_arm_condition_code(), from here:
https://bugs.launchpad.net/gcc-linaro/+bug/689887

The report was on a 4.5 based compiler, but I verified it also occurs on
current trunk, under Thumb-2.

The problem as I found, happens in RTL ifcvt after the combine pass.
ifcvt.c:noce_emit_cmove() creates this:
(insn 58 57 59 4 (parallel [
            (set (reg:CC_NCV 24 cc)
                (compare:CC_NCV (reg:DI 140)
                    (reg:DI 147)))
            (clobber (scratch:SI))
        ]) p.c:13 202 {*arm_cmpdi_insn}
     (nil))

(insn 59 58 60 4 (set (reg:SI 146)
	(if_then_else:SI (gt (reg:CC_NCV 24 cc)
                (const_int 0 [0]))
            (reg:SI 145)
            (reg:SI 144))) p.c:13 701 {*thumb2_movsicc_insn}
     (nil))

which is wrong; the GT comparison requires testing of the Z, C, and V
flags, which CC_NCV does not satisfy. The ICE happens later when the asm
emitting actually hits assert fail in get_arm_condition_code().

This patch fixes this by stratifying the "arm_comparison_operator"
predicate, by leveraging the current logic in get_arm_condition_code().
I abstracted out the body into a new arm_comparison_to_cond_code()
function, which returns an invalid code instead of assert fail, and used
it to add a check to the comparison RTX in "arm_comparison_operator".
This way, invalid comparisons like the above would be blocked from emitting.

Tests were run with no regressions found. Okay for trunk?

Thanks,
Chung-Lin

2011-01-12  Chung-Lin Tang  <cltang@codesourcery.com>

        * config/arm/arm.h (enum arm_cond_code): Add ARM_COND_INVALID.
        * config/arm/arm.c (arm_comparison_to_cond_code): Renamed from
        get_arm_condition_code(), change gcc_unreachable()s to return
        ARM_COND_INVALID.
        (get_arm_condition_code): Implement by calling
        arm_comparison_to_cond_code(), with result test by gcc_assert().
        (arm_valid_comparison_p): New function to test for valid ARM
        comparison RTX.
        * config/arm/arm-protos.h (arm_valid_comparison_p): Add
	prototype.
        * config/arm/predicates.md (arm_comparison_operator): Add
	additional check based on arm_valid_comparison_p().

        testsuite/
        * gcc.target/arm/20110112-1.c: New test.




[-- Attachment #2: arm-cmp.diff --]
[-- Type: text/x-patch, Size: 5623 bytes --]

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 168663)
+++ config/arm/arm.c	(working copy)
@@ -90,6 +90,7 @@
 static bool arm_print_operand_punct_valid_p (unsigned char code);
 static const char *fp_const_from_val (REAL_VALUE_TYPE *);
 static arm_cc get_arm_condition_code (rtx);
+static arm_cc arm_comparison_to_cond_code (rtx);
 static HOST_WIDE_INT int_log2 (HOST_WIDE_INT);
 static rtx is_jump_table (rtx);
 static const char *output_multi_immediate (rtx *, const char *, const char *,
@@ -16942,6 +16943,23 @@
 static enum arm_cond_code
 get_arm_condition_code (rtx comparison)
 {
+  enum arm_cond_code code = arm_comparison_to_cond_code (comparison);
+  gcc_assert (code != ARM_COND_INVALID);
+  return code;
+}
+
+/* Tests if the comparison RTX is valid for ARM. */
+bool
+arm_valid_comparison_p (rtx comparison)
+{
+  return (arm_comparison_to_cond_code (comparison) != ARM_COND_INVALID);
+}
+
+/* Function that maps the relation between comparison RTX
+   vs. ARM condition codes, */
+static enum arm_cond_code
+arm_comparison_to_cond_code (rtx comparison)
+{
   enum machine_mode mode = GET_MODE (XEXP (comparison, 0));
   enum arm_cond_code code;
   enum rtx_code comp_code = GET_CODE (comparison);
@@ -16977,7 +16995,7 @@
 	case EQ: return ARM_EQ;
 	case GE: return ARM_PL;
 	case LT: return ARM_MI;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_Zmode:
@@ -16985,7 +17003,7 @@
 	{
 	case NE: return ARM_NE;
 	case EQ: return ARM_EQ;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_Nmode:
@@ -16993,7 +17011,7 @@
 	{
 	case NE: return ARM_MI;
 	case EQ: return ARM_PL;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CCFPEmode:
@@ -17018,7 +17036,7 @@
 	  /* UNEQ and LTGT do not have a representation.  */
 	case UNEQ: /* Fall through.  */
 	case LTGT: /* Fall through.  */
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_SWPmode:
@@ -17034,7 +17052,7 @@
 	case GTU: return ARM_CC;
 	case LEU: return ARM_CS;
 	case LTU: return ARM_HI;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_Cmode:
@@ -17042,7 +17060,7 @@
 	{
 	case LTU: return ARM_CS;
 	case GEU: return ARM_CC;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_CZmode:
@@ -17054,7 +17072,7 @@
 	case GTU: return ARM_HI;
 	case LEU: return ARM_LS;
 	case LTU: return ARM_CC;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CC_NCVmode:
@@ -17064,7 +17082,7 @@
 	case LT: return ARM_LT;
 	case GEU: return ARM_CS;
 	case LTU: return ARM_CC;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
     case CCmode:
@@ -17080,10 +17098,10 @@
 	case GTU: return ARM_HI;
 	case LEU: return ARM_LS;
 	case LTU: return ARM_CC;
-	default: gcc_unreachable ();
+	default: return ARM_COND_INVALID;
 	}
 
-    default: gcc_unreachable ();
+    default: return ARM_COND_INVALID;
     }
 }
 
Index: config/arm/arm.h
===================================================================
--- config/arm/arm.h	(revision 168663)
+++ config/arm/arm.h	(working copy)
@@ -147,7 +147,8 @@
 typedef enum arm_cond_code
 {
   ARM_EQ = 0, ARM_NE, ARM_CS, ARM_CC, ARM_MI, ARM_PL, ARM_VS, ARM_VC,
-  ARM_HI, ARM_LS, ARM_GE, ARM_LT, ARM_GT, ARM_LE, ARM_AL, ARM_NV
+  ARM_HI, ARM_LS, ARM_GE, ARM_LT, ARM_GT, ARM_LE, ARM_AL, ARM_NV,
+  ARM_COND_INVALID
 }
 arm_cc;
 
Index: config/arm/arm-protos.h
===================================================================
--- config/arm/arm-protos.h	(revision 168663)
+++ config/arm/arm-protos.h	(working copy)
@@ -107,6 +107,7 @@
 extern enum machine_mode arm_select_cc_mode (RTX_CODE, rtx, rtx);
 extern enum machine_mode arm_select_dominance_cc_mode (rtx, rtx,
 						       HOST_WIDE_INT);
+extern bool arm_valid_comparison_p (rtx);
 extern rtx arm_gen_compare_reg (RTX_CODE, rtx, rtx);
 extern rtx arm_gen_return_addr_mask (void);
 extern void arm_reload_in_hi (rtx *);
Index: config/arm/predicates.md
===================================================================
--- config/arm/predicates.md	(revision 168663)
+++ config/arm/predicates.md	(working copy)
@@ -242,10 +242,11 @@
 ;; True for integer comparisons and, if FP is active, for comparisons
 ;; other than LTGT or UNEQ.
 (define_special_predicate "arm_comparison_operator"
-  (ior (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu")
-       (and (match_test "TARGET_32BIT && TARGET_HARD_FLOAT
-			 && (TARGET_FPA || TARGET_VFP)")
-            (match_code "unordered,ordered,unlt,unle,unge,ungt"))))
+  (and (ior (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu")
+            (and (match_test "TARGET_32BIT && TARGET_HARD_FLOAT
+                              && (TARGET_FPA || TARGET_VFP)")
+                 (match_code "unordered,ordered,unlt,unle,unge,ungt")))
+       (match_test "arm_valid_comparison_p (op)")))
 
 (define_special_predicate "lt_ge_comparison_operator"
   (match_code "lt,ge"))
Index: testsuite/gcc.target/arm/20110112-1.c
===================================================================
--- testsuite/gcc.target/arm/20110112-1.c	(revision 0)
+++ testsuite/gcc.target/arm/20110112-1.c	(revision 0)
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O -march=armv7-a -mthumb" } */
+
+#include <limits.h>
+
+void small (char *dst)
+{
+  while (1)
+    {
+      long long y = (long long)(*dst) << 8;
+
+      if (y < INT_MIN)
+	*((int *) dst) = 0;
+      else
+	*((int *) dst) = 1;
+    }
+}

Re: [patch, ARM] ICE in get_arm_condition_code()

[Ramana Radhakrishnan]

On Thu, Jan 13, 2011 at 12:24 AM, Chung-Lin Tang
<cltang@codesourcery.com> wrote:
> Hi
> The report was on a 4.5 based compiler, but I verified it also occurs on
> current trunk, under Thumb-2.
>@@ -16942,6 +16943,23 @@
> static enum arm_cond_code
> get_arm_condition_code (rtx comparison)
> {
>+  enum arm_cond_code code = arm_comparison_to_cond_code (comparison);
>+  gcc_assert (code != ARM_COND_INVALID);
>+  return code;
>+}
>+
>+/* Tests if the comparison RTX is valid for ARM. */

Minor nits really that I spotted .

2 spaces after the end of a '.' before the end of comment.

>+/* Function that maps the relation between comparison RTX
>+   vs. ARM condition codes, */

s/,/. and the 2 spaces after the . rule apply.



cheers
Ramana

Re: [patch, ARM] ICE in get_arm_condition_code()

[Chung-Lin Tang]

Ping.
(I'll fix the minor nits before actually committing, thanks Ramana :)

On 2011/1/13 08:24, Chung-Lin Tang wrote:
> Hi,
> there's an ICE in arm.c:get_arm_condition_code(), from here:
> https://bugs.launchpad.net/gcc-linaro/+bug/689887
> 
> The report was on a 4.5 based compiler, but I verified it also occurs on
> current trunk, under Thumb-2.
> 
> The problem as I found, happens in RTL ifcvt after the combine pass.
> ifcvt.c:noce_emit_cmove() creates this:
> (insn 58 57 59 4 (parallel [
>             (set (reg:CC_NCV 24 cc)
>                 (compare:CC_NCV (reg:DI 140)
>                     (reg:DI 147)))
>             (clobber (scratch:SI))
>         ]) p.c:13 202 {*arm_cmpdi_insn}
>      (nil))
> 
> (insn 59 58 60 4 (set (reg:SI 146)
> 	(if_then_else:SI (gt (reg:CC_NCV 24 cc)
>                 (const_int 0 [0]))
>             (reg:SI 145)
>             (reg:SI 144))) p.c:13 701 {*thumb2_movsicc_insn}
>      (nil))
> 
> which is wrong; the GT comparison requires testing of the Z, C, and V
> flags, which CC_NCV does not satisfy. The ICE happens later when the asm
> emitting actually hits assert fail in get_arm_condition_code().
> 
> This patch fixes this by stratifying the "arm_comparison_operator"
> predicate, by leveraging the current logic in get_arm_condition_code().
> I abstracted out the body into a new arm_comparison_to_cond_code()
> function, which returns an invalid code instead of assert fail, and used
> it to add a check to the comparison RTX in "arm_comparison_operator".
> This way, invalid comparisons like the above would be blocked from emitting.
> 
> Tests were run with no regressions found. Okay for trunk?
> 
> Thanks,
> Chung-Lin
> 
> 2011-01-12  Chung-Lin Tang  <cltang@codesourcery.com>
> 
>         * config/arm/arm.h (enum arm_cond_code): Add ARM_COND_INVALID.
>         * config/arm/arm.c (arm_comparison_to_cond_code): Renamed from
>         get_arm_condition_code(), change gcc_unreachable()s to return
>         ARM_COND_INVALID.
>         (get_arm_condition_code): Implement by calling
>         arm_comparison_to_cond_code(), with result test by gcc_assert().
>         (arm_valid_comparison_p): New function to test for valid ARM
>         comparison RTX.
>         * config/arm/arm-protos.h (arm_valid_comparison_p): Add
> 	prototype.
>         * config/arm/predicates.md (arm_comparison_operator): Add
> 	additional check based on arm_valid_comparison_p().
> 
>         testsuite/
>         * gcc.target/arm/20110112-1.c: New test.
> 
> 
> 

Ping Re: [patch, ARM] ICE in get_arm_condition_code()

[Chung-Lin Tang]

Ping.

On 2011/1/26 11:09 AM, Chung-Lin Tang wrote:
> Ping.
> (I'll fix the minor nits before actually committing, thanks Ramana :)
> 
> On 2011/1/13 08:24, Chung-Lin Tang wrote:
>> Hi,
>> there's an ICE in arm.c:get_arm_condition_code(), from here:
>> https://bugs.launchpad.net/gcc-linaro/+bug/689887
>>
>> The report was on a 4.5 based compiler, but I verified it also occurs on
>> current trunk, under Thumb-2.
>>
>> The problem as I found, happens in RTL ifcvt after the combine pass.
>> ifcvt.c:noce_emit_cmove() creates this:
>> (insn 58 57 59 4 (parallel [
>>             (set (reg:CC_NCV 24 cc)
>>                 (compare:CC_NCV (reg:DI 140)
>>                     (reg:DI 147)))
>>             (clobber (scratch:SI))
>>         ]) p.c:13 202 {*arm_cmpdi_insn}
>>      (nil))
>>
>> (insn 59 58 60 4 (set (reg:SI 146)
>> 	(if_then_else:SI (gt (reg:CC_NCV 24 cc)
>>                 (const_int 0 [0]))
>>             (reg:SI 145)
>>             (reg:SI 144))) p.c:13 701 {*thumb2_movsicc_insn}
>>      (nil))
>>
>> which is wrong; the GT comparison requires testing of the Z, C, and V
>> flags, which CC_NCV does not satisfy. The ICE happens later when the asm
>> emitting actually hits assert fail in get_arm_condition_code().
>>
>> This patch fixes this by stratifying the "arm_comparison_operator"
>> predicate, by leveraging the current logic in get_arm_condition_code().
>> I abstracted out the body into a new arm_comparison_to_cond_code()
>> function, which returns an invalid code instead of assert fail, and used
>> it to add a check to the comparison RTX in "arm_comparison_operator".
>> This way, invalid comparisons like the above would be blocked from emitting.
>>
>> Tests were run with no regressions found. Okay for trunk?
>>
>> Thanks,
>> Chung-Lin
>>
>> 2011-01-12  Chung-Lin Tang  <cltang@codesourcery.com>
>>
>>         * config/arm/arm.h (enum arm_cond_code): Add ARM_COND_INVALID.
>>         * config/arm/arm.c (arm_comparison_to_cond_code): Renamed from
>>         get_arm_condition_code(), change gcc_unreachable()s to return
>>         ARM_COND_INVALID.
>>         (get_arm_condition_code): Implement by calling
>>         arm_comparison_to_cond_code(), with result test by gcc_assert().
>>         (arm_valid_comparison_p): New function to test for valid ARM
>>         comparison RTX.
>>         * config/arm/arm-protos.h (arm_valid_comparison_p): Add
>> 	prototype.
>>         * config/arm/predicates.md (arm_comparison_operator): Add
>> 	additional check based on arm_valid_comparison_p().
>>
>>         testsuite/
>>         * gcc.target/arm/20110112-1.c: New test.
>>
>>
>>
> 

Ping^3 Re: [patch, ARM] ICE in get_arm_condition_code()

[Chung-Lin Tang]

Ping.

On 2011/3/7 10:10 PM, Chung-Lin Tang wrote:
> Ping.
> 
> On 2011/1/26 11:09 AM, Chung-Lin Tang wrote:
>> Ping.
>> (I'll fix the minor nits before actually committing, thanks Ramana :)
>>
>> On 2011/1/13 08:24, Chung-Lin Tang wrote:
>>> Hi,
>>> there's an ICE in arm.c:get_arm_condition_code(), from here:
>>> https://bugs.launchpad.net/gcc-linaro/+bug/689887
>>>
>>> The report was on a 4.5 based compiler, but I verified it also occurs on
>>> current trunk, under Thumb-2.
>>>
>>> The problem as I found, happens in RTL ifcvt after the combine pass.
>>> ifcvt.c:noce_emit_cmove() creates this:
>>> (insn 58 57 59 4 (parallel [
>>>             (set (reg:CC_NCV 24 cc)
>>>                 (compare:CC_NCV (reg:DI 140)
>>>                     (reg:DI 147)))
>>>             (clobber (scratch:SI))
>>>         ]) p.c:13 202 {*arm_cmpdi_insn}
>>>      (nil))
>>>
>>> (insn 59 58 60 4 (set (reg:SI 146)
>>> 	(if_then_else:SI (gt (reg:CC_NCV 24 cc)
>>>                 (const_int 0 [0]))
>>>             (reg:SI 145)
>>>             (reg:SI 144))) p.c:13 701 {*thumb2_movsicc_insn}
>>>      (nil))
>>>
>>> which is wrong; the GT comparison requires testing of the Z, C, and V
>>> flags, which CC_NCV does not satisfy. The ICE happens later when the asm
>>> emitting actually hits assert fail in get_arm_condition_code().
>>>
>>> This patch fixes this by stratifying the "arm_comparison_operator"
>>> predicate, by leveraging the current logic in get_arm_condition_code().
>>> I abstracted out the body into a new arm_comparison_to_cond_code()
>>> function, which returns an invalid code instead of assert fail, and used
>>> it to add a check to the comparison RTX in "arm_comparison_operator".
>>> This way, invalid comparisons like the above would be blocked from emitting.
>>>
>>> Tests were run with no regressions found. Okay for trunk?
>>>
>>> Thanks,
>>> Chung-Lin
>>>
>>> 2011-01-12  Chung-Lin Tang  <cltang@codesourcery.com>
>>>
>>>         * config/arm/arm.h (enum arm_cond_code): Add ARM_COND_INVALID.
>>>         * config/arm/arm.c (arm_comparison_to_cond_code): Renamed from
>>>         get_arm_condition_code(), change gcc_unreachable()s to return
>>>         ARM_COND_INVALID.
>>>         (get_arm_condition_code): Implement by calling
>>>         arm_comparison_to_cond_code(), with result test by gcc_assert().
>>>         (arm_valid_comparison_p): New function to test for valid ARM
>>>         comparison RTX.
>>>         * config/arm/arm-protos.h (arm_valid_comparison_p): Add
>>> 	prototype.
>>>         * config/arm/predicates.md (arm_comparison_operator): Add
>>> 	additional check based on arm_valid_comparison_p().
>>>
>>>         testsuite/
>>>         * gcc.target/arm/20110112-1.c: New test.
>>>
>>>
>>>
>>
>