| Message ID | 562FBD14.3050105@arm.com |
|---|---|
| State | Accepted |
| Commit | 7263fa9ff190a311c6fefc4aafb374437d1020ae |
| Headers | show |
On Tue, Oct 27, 2015 at 06:06:12PM +0000, Kyrill Tkachov wrote: > Hi all, > > This is another RTL checking error occuring in the splitting condition of the > mov-immediate patterns. We take a REGNO of operands[0] which is a > nonimmediate_operand. Since the immediate splitting code only makes sense > when the destination is a register, we should be guarding that condition on > REG_P (operands[0]). > > The reported error occurs on the *movdi_aarch64 pattern but I see the same > vulnerability in the *movsi_aarch64 pattern, although I wasn't able to get it > to trigger an ICE. > > This patch adds a REG_P check on the splitting condition of both. The > testcase (taken from the BZ for PR 68102 and with an #if 1 removed)now > compiles fine on an aarch64 compiler with RTL checking enabled. > Bootstrapped and tested on aarch64-linux with RTL checking enabled. > > Ok for trunk? OK. > Thanks, > Kyrill > > The BZ says this occurs on the GCC 5 branch but I don't have a checking > compiler from that branch yet. I'll be investigating whether to backport this > patch there in the meantime. Sounds good to me. Thanks, James > > 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68102 > * config/aarch64/aarch64.md (*movsi_aarch64): Check that > operands[0] is a reg before taking its REGNO in split condition. > (*movdi_aarch64): Likewise. > > 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/68102 > * gcc.target/aarch64/pr68102_1.c: New test.
Hi James, On 27/10/15 18:26, James Greenhalgh wrote: > On Tue, Oct 27, 2015 at 06:06:12PM +0000, Kyrill Tkachov wrote: >> Hi all, >> >> This is another RTL checking error occuring in the splitting condition of the >> mov-immediate patterns. We take a REGNO of operands[0] which is a >> nonimmediate_operand. Since the immediate splitting code only makes sense >> when the destination is a register, we should be guarding that condition on >> REG_P (operands[0]). >> >> The reported error occurs on the *movdi_aarch64 pattern but I see the same >> vulnerability in the *movsi_aarch64 pattern, although I wasn't able to get it >> to trigger an ICE. >> >> This patch adds a REG_P check on the splitting condition of both. The >> testcase (taken from the BZ for PR 68102 and with an #if 1 removed)now >> compiles fine on an aarch64 compiler with RTL checking enabled. >> Bootstrapped and tested on aarch64-linux with RTL checking enabled. >> >> Ok for trunk? > OK. > >> Thanks, >> Kyrill >> >> The BZ says this occurs on the GCC 5 branch but I don't have a checking >> compiler from that branch yet. I'll be investigating whether to backport this >> patch there in the meantime. > Sounds good to me. So I reproduced the checking ICE on the GCC 5 and the patch applies cleanly there and fixes it. So ok to commit it there after a bootstrap and test on that branch? Thanks, Kyrill > Thanks, > James > >> 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/68102 >> * config/aarch64/aarch64.md (*movsi_aarch64): Check that >> operands[0] is a reg before taking its REGNO in split condition. >> (*movdi_aarch64): Likewise. >> >> 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> >> >> PR target/68102 >> * gcc.target/aarch64/pr68102_1.c: New test.
On Wed, Oct 28, 2015 at 09:49:59AM +0000, Kyrill Tkachov wrote: > Hi James, > > On 27/10/15 18:26, James Greenhalgh wrote: > >On Tue, Oct 27, 2015 at 06:06:12PM +0000, Kyrill Tkachov wrote: > >>Hi all, > >> > >>This is another RTL checking error occuring in the splitting condition of the > >>mov-immediate patterns. We take a REGNO of operands[0] which is a > >>nonimmediate_operand. Since the immediate splitting code only makes sense > >>when the destination is a register, we should be guarding that condition on > >>REG_P (operands[0]). > >> > >>The reported error occurs on the *movdi_aarch64 pattern but I see the same > >>vulnerability in the *movsi_aarch64 pattern, although I wasn't able to get it > >>to trigger an ICE. > >> > >>This patch adds a REG_P check on the splitting condition of both. The > >>testcase (taken from the BZ for PR 68102 and with an #if 1 removed)now > >>compiles fine on an aarch64 compiler with RTL checking enabled. > >>Bootstrapped and tested on aarch64-linux with RTL checking enabled. > >> > >>Ok for trunk? > >OK. > > > >>Thanks, > >>Kyrill > >> > >>The BZ says this occurs on the GCC 5 branch but I don't have a checking > >>compiler from that branch yet. I'll be investigating whether to backport this > >>patch there in the meantime. > >Sounds good to me. > > So I reproduced the checking ICE on the GCC 5 and the patch applies > cleanly there and fixes it. > > So ok to commit it there after a bootstrap and test on that branch? Yes, please. Thanks, James
commit 60e037bdc67949abe2589a91a80afd67c9b13926
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Tue Oct 27 12:18:22 2015 +0000
[AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 77df46f..0cb64ee 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1031,7 +1031,7 @@ (define_insn_and_split "*movsi_aarch64"
fmov\\t%w0, %s1
fmov\\t%s0, %s1"
"CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), SImode)
- && GP_REGNUM_P (REGNO (operands[0]))"
+ && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
[(const_int 0)]
"{
aarch64_expand_mov_immediate (operands[0], operands[1]);
@@ -1064,7 +1064,7 @@ (define_insn_and_split "*movdi_aarch64"
fmov\\t%d0, %d1
movi\\t%d0, %1"
"(CONST_INT_P (operands[1]) && !aarch64_move_imm (INTVAL (operands[1]), DImode))
- && GP_REGNUM_P (REGNO (operands[0]))"
+ && REG_P (operands[0]) && GP_REGNUM_P (REGNO (operands[0]))"
[(const_int 0)]
"{
aarch64_expand_mov_immediate (operands[0], operands[1]);
diff --git a/gcc/testsuite/gcc.target/aarch64/pr68102_1.c b/gcc/testsuite/gcc.target/aarch64/pr68102_1.c
new file mode 100644
index 0000000..3193b27
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr68102_1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef __Float64x1_t float64x1_t;
+
+typedef long int64_t;
+
+extern int64_t bar (float64x1_t f);
+
+int
+foo (void)
+{
+ float64x1_t f = { 3.14159265358979311599796346854 };
+ int64_t c = 0x400921FB54442D18;
+ int64_t r;
+ r = bar (f);
+ return r == c;
+}
Hi all, This is another RTL checking error occuring in the splitting condition of the mov-immediate patterns. We take a REGNO of operands[0] which is a nonimmediate_operand. Since the immediate splitting code only makes sense when the destination is a register, we should be guarding that condition on REG_P (operands[0]). The reported error occurs on the *movdi_aarch64 pattern but I see the same vulnerability in the *movsi_aarch64 pattern, although I wasn't able to get it to trigger an ICE. This patch adds a REG_P check on the splitting condition of both. The testcase (taken from the BZ for PR 68102 and with an #if 1 removed)now compiles fine on an aarch64 compiler with RTL checking enabled. Bootstrapped and tested on aarch64-linux with RTL checking enabled. Ok for trunk? Thanks, Kyrill The BZ says this occurs on the GCC 5 branch but I don't have a checking compiler from that branch yet. I'll be investigating whether to backport this patch there in the meantime. 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/68102 * config/aarch64/aarch64.md (*movsi_aarch64): Check that operands[0] is a reg before taking its REGNO in split condition. (*movdi_aarch64): Likewise. 2015-10-27 Kyrylo Tkachov <kyrylo.tkachov@arm.com> PR target/68102 * gcc.target/aarch64/pr68102_1.c: New test.