| Message ID | 55487DA8.1050709@arm.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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))
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.