[PATCH][ARM] Fix PR 65955: Do not take REGNO on non-REG operand in movcond_addsi

[PATCH][ARM] Fix PR 65955: Do not take REGNO on non-REG operand in movcond_addsi

[Kyrill Tkachov]

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

Hi all,

As the PR says, the movcond_addsi pattern takes the REGNO of an operand that may be a CONST_INT.
The fix for that is rather simple (perhaps even obvious?).

Unfortunately the testcase is in ada, and I'm not sure how to integrate this into the testsuite.
Does anyone know how to add an ada testcase?

The patch that introduced this faulty check was added for GCC 5 and backported to 4.9, so this patch
should be backported everywhere as well.

I'll be testing it on those branches shortly.

Tested on arm-none-eabi. Bootstrapped on arm-none-linux-gnueabihf with arm and thumb default mode.
I think this is the right thing to do in any case since the current code is clearly doing the wrong
thing if operands[2] is a CONST_INT.

Ok for trunk?

Thanks,
Kyrill

P.S. I'm attaching the patch with a larger than usual context so that the definition of operands[2]
can be seen more clearly.

2015-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/65955
     * config/arm/arm.md (movcond_addsi): Check that operands[2] is a
     REG before taking its REGNO.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-movcond-addsi-regp.patch --]
[-- Type: text/x-patch; name=arm-movcond-addsi-regp.patch, Size: 2308 bytes --]

commit e88f515909e185e7add7429b11672ccc3ad17be9
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Fri May 1 10:01:13 2015 +0100

    [ARM] PR 65955: check for REG_P before taking REGNO in movcond_addsi

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index b2f6b4f..e439e7a 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -9413,51 +9413,51 @@ (define_insn_and_split "movcond_addsi"
 	 (match_operator 5 "comparison_operator"
 	  [(plus:SI (match_operand:SI 3 "s_register_operand" "r,r,r")
 	            (match_operand:SI 4 "arm_add_operand" "rIL,rIL,rIL"))
             (const_int 0)])
 	 (match_operand:SI 1 "arm_rhs_operand" "rI,rPy,r")
 	 (match_operand:SI 2 "arm_rhs_operand" "rI,rPy,r")))
    (clobber (reg:CC CC_REGNUM))]
    "TARGET_32BIT"
    "#"
    "&& reload_completed"
   [(set (reg:CC_NOOV CC_REGNUM)
 	(compare:CC_NOOV
 	 (plus:SI (match_dup 3)
 		  (match_dup 4))
 	 (const_int 0)))
    (set (match_dup 0) (match_dup 1))
    (cond_exec (match_dup 6)
 	      (set (match_dup 0) (match_dup 2)))]
   "
   {
     machine_mode mode = SELECT_CC_MODE (GET_CODE (operands[5]),
 					     operands[3], operands[4]);
     enum rtx_code rc = GET_CODE (operands[5]);
     operands[6] = gen_rtx_REG (mode, CC_REGNUM);
     gcc_assert (!(mode == CCFPmode || mode == CCFPEmode));
-    if (REGNO (operands[2]) != REGNO (operands[0]))
+    if (!REG_P (operands[2]) || REGNO (operands[2]) != REGNO (operands[0]))
       rc = reverse_condition (rc);
     else
       std::swap (operands[1], operands[2]);
 
     operands[6] = gen_rtx_fmt_ee (rc, VOIDmode, operands[6], const0_rtx);
   }
   "
   [(set_attr "conds" "clob")
    (set_attr "enabled_for_depr_it" "no,yes,yes")
    (set_attr "type" "multiple")]
 )
 
 (define_insn "movcond"
   [(set (match_operand:SI 0 "s_register_operand" "=r,r,r")
 	(if_then_else:SI
 	 (match_operator 5 "arm_comparison_operator"
 	  [(match_operand:SI 3 "s_register_operand" "r,r,r")
 	   (match_operand:SI 4 "arm_add_operand" "rIL,rIL,rIL")])
 	 (match_operand:SI 1 "arm_rhs_operand" "0,rI,?rI")
 	 (match_operand:SI 2 "arm_rhs_operand" "rI,0,rI")))
    (clobber (reg:CC CC_REGNUM))]
   "TARGET_ARM"
   "*
   if (GET_CODE (operands[5]) == LT
       && (operands[4] == const0_rtx))

Re: [PATCH][ARM] Fix PR 65955: Do not take REGNO on non-REG operand in movcond_addsi

[Kyrill Tkachov]

On 05/05/15 09:22, Kyrill Tkachov wrote:
> Hi all,
>
> As the PR says, the movcond_addsi pattern takes the REGNO of an operand that may be a CONST_INT.
> The fix for that is rather simple (perhaps even obvious?).
>
> Unfortunately the testcase is in ada, and I'm not sure how to integrate this into the testsuite.
> Does anyone know how to add an ada testcase?

As Tom has investigated in the bugzilla entry for this, the failure can
be reproduced in the testsuite at gfortran.dg/pr43984.f90 with
--enable-checking=yes,rtl.

So, this patch fixes that ICE, so I think no new testcase should be required for this.
So, is this patch ok for trunk as is then?

Thanks,
Kyrill

>
> The patch that introduced this faulty check was added for GCC 5 and backported to 4.9, so this patch
> should be backported everywhere as well.
>
> I'll be testing it on those branches shortly.
>
> Tested on arm-none-eabi. Bootstrapped on arm-none-linux-gnueabihf with arm and thumb default mode.
> I think this is the right thing to do in any case since the current code is clearly doing the wrong
> thing if operands[2] is a CONST_INT.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> P.S. I'm attaching the patch with a larger than usual context so that the definition of operands[2]
> can be seen more clearly.
>
> 2015-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       PR target/65955
>       * config/arm/arm.md (movcond_addsi): Check that operands[2] is a
>       REG before taking its REGNO.

Re: [PATCH][ARM] Fix PR 65955: Do not take REGNO on non-REG operand in movcond_addsi

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00284.html

Thanks,
Kyrill

On 05/05/15 11:50, Kyrill Tkachov wrote:
> On 05/05/15 09:22, Kyrill Tkachov wrote:
>> Hi all,
>>
>> As the PR says, the movcond_addsi pattern takes the REGNO of an operand that may be a CONST_INT.
>> The fix for that is rather simple (perhaps even obvious?).
>>
>> Unfortunately the testcase is in ada, and I'm not sure how to integrate this into the testsuite.
>> Does anyone know how to add an ada testcase?
> As Tom has investigated in the bugzilla entry for this, the failure can
> be reproduced in the testsuite at gfortran.dg/pr43984.f90 with
> --enable-checking=yes,rtl.
>
> So, this patch fixes that ICE, so I think no new testcase should be required for this.
> So, is this patch ok for trunk as is then?
>
> Thanks,
> Kyrill
>
>> The patch that introduced this faulty check was added for GCC 5 and backported to 4.9, so this patch
>> should be backported everywhere as well.
>>
>> I'll be testing it on those branches shortly.
>>
>> Tested on arm-none-eabi. Bootstrapped on arm-none-linux-gnueabihf with arm and thumb default mode.
>> I think this is the right thing to do in any case since the current code is clearly doing the wrong
>> thing if operands[2] is a CONST_INT.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> P.S. I'm attaching the patch with a larger than usual context so that the definition of operands[2]
>> can be seen more clearly.
>>
>> 2015-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>        PR target/65955
>>        * config/arm/arm.md (movcond_addsi): Check that operands[2] is a
>>        REG before taking its REGNO.

Re: [PATCH][ARM] Fix PR 65955: Do not take REGNO on non-REG operand in movcond_addsi

[Ramana Radhakrishnan]

On 05/05/15 09:22, Kyrill Tkachov wrote:
> Hi all,
>
> As the PR says, the movcond_addsi pattern takes the REGNO of an operand that may be a CONST_INT.
> The fix for that is rather simple (perhaps even obvious?).
>
> Unfortunately the testcase is in ada, and I'm not sure how to integrate this into the testsuite.
> Does anyone know how to add an ada testcase?
>
> The patch that introduced this faulty check was added for GCC 5 and backported to 4.9, so this patch
> should be backported everywhere as well.
>
> I'll be testing it on those branches shortly.
>
> Tested on arm-none-eabi. Bootstrapped on arm-none-linux-gnueabihf with arm and thumb default mode.
> I think this is the right thing to do in any case since the current code is clearly doing the wrong
> thing if operands[2] is a CONST_INT.
>
> Ok for trunk?
>

> Thanks,
> Kyrill
>
> P.S. I'm attaching the patch with a larger than usual context so that the definition of operands[2]
> can be seen more clearly.
>
> 2015-05-05  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       PR target/65955
>       * config/arm/arm.md (movcond_addsi): Check that operands[2] is a
>       REG before taking its REGNO.
>




OK for trunk and all release branches if no regressions.

Sorry to have missed this email earlier.

Ramana