[PATCH][AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters

[PATCH][AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters

[Kyrill Tkachov]

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

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-mov-imm-check.patch --]
[-- Type: text/x-patch; name=aarch64-mov-imm-check.patch, Size: 1794 bytes --]

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;
+}

Re: [PATCH][AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters

[James Greenhalgh]

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.

Re: [PATCH][AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters

[Kyrill Tkachov]

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.

Re: [PATCH][AArch64] PR 68102: Check that operand is REG before checking the REGNO in mov-immediate splitters

[James Greenhalgh]

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