public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register
@ 2016-11-16 16:57 Kyrill Tkachov
  2016-11-23 14:17 ` Kyrill Tkachov
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Kyrill Tkachov @ 2016-11-16 16:57 UTC (permalink / raw)
  To: GCC Patches; +Cc: Marcus Shawcroft, Richard Earnshaw, James Greenhalgh

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

Hi all,

As the PR says we have an RTL checking failure that occurs when building libgcc for aarch64.
The expander code for addsi3 takes the REGNO of a SUBREG in operands[1]. The three operands
in the failing case are:
{(reg:SI 78), (subreg:SI (reg:DI 77) 0), (subreg:SI (reg:DI 73 [ ivtmp.9 ]) 0)}

According to the documentation of register_operand (which is the predicate for operands[1]),
operands[1] can be a REG or a SUBREG. If it's a subreg it may also contain a MEM before reload
(because it is guaranteed to be reloaded into a register later). Anyway, the bottom line is that
we have to be careful when taking REGNO of expressions during expand-time.

This patch extracts the inner rtx in case we have a SUBREG and checks that it's a REG before
checking its REGNO.

Bootstrapped and tested on aarch64-none-linux-gnu. Tested aarch64-none-elf with RTL checking enabled
(without this patch that doesn't build).

Ok for trunk?
Thanks,
Kyrill

2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/78362
     * config/aarch64/aarch64.md (add<mode>3): Extract inner expression
     from a subreg in operands[1] and don't call REGNO on a non-reg expression
         when deciding to force operands[2] into a reg.

2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     PR target/78362
     * gcc.c-torture/compile/pr78362.c: New test.

[-- Attachment #2: aarch64-rtl-check.patch --]
[-- Type: text/x-patch, Size: 1407 bytes --]

commit 068224c568d6f06f68512f12ecebea8bfc873fe9
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Nov 15 14:52:33 2016 +0000

    [AArch64] PR target/78362: Make sure to only take REGNO of a register

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 9e5eee9..1dcb6b2 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -1611,11 +1611,15 @@ (define_expand "add<mode>3"
 	      (match_operand:GPI 2 "aarch64_pluslong_operand" "")))]
   ""
 {
+  /* If operands[1] is a subreg extract the inner RTX.  */
+  rtx op1 = REG_P (operands[1]) ? operands[1] : SUBREG_REG (operands[1]);
+
   /* If the constant is too large for a single instruction and isn't frame
      based, split off the immediate so it is available for CSE.  */
   if (!aarch64_plus_immediate (operands[2], <MODE>mode)
       && can_create_pseudo_p ()
-      && !REGNO_PTR_FRAME_P (REGNO (operands[1])))
+      && (!REG_P (op1)
+	 || !REGNO_PTR_FRAME_P (REGNO (op1))))
     operands[2] = force_reg (<MODE>mode, operands[2]);
 })
 
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr78362.c b/gcc/testsuite/gcc.c-torture/compile/pr78362.c
new file mode 100644
index 0000000..66eea7d
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr78362.c
@@ -0,0 +1,11 @@
+/* PR target/78362.  */
+
+long a;
+
+void
+foo (void)
+{
+  for (;; a--)
+    if ((int) a)
+      break;
+}

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register
  2016-11-16 16:57 [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register Kyrill Tkachov
@ 2016-11-23 14:17 ` Kyrill Tkachov
  2016-11-30 10:33   ` Kyrill Tkachov
  2016-11-30 11:50 ` Ramana Radhakrishnan
  2016-11-30 11:55 ` James Greenhalgh
  2 siblings, 1 reply; 5+ messages in thread
From: Kyrill Tkachov @ 2016-11-23 14:17 UTC (permalink / raw)
  To: GCC Patches; +Cc: Marcus Shawcroft, Richard Earnshaw, James Greenhalgh


Ping.
https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01664.html

Thanks,
Kyrill

On 16/11/16 16:57, Kyrill Tkachov wrote:
> Hi all,
>
> As the PR says we have an RTL checking failure that occurs when building libgcc for aarch64.
> The expander code for addsi3 takes the REGNO of a SUBREG in operands[1]. The three operands
> in the failing case are:
> {(reg:SI 78), (subreg:SI (reg:DI 77) 0), (subreg:SI (reg:DI 73 [ ivtmp.9 ]) 0)}
>
> According to the documentation of register_operand (which is the predicate for operands[1]),
> operands[1] can be a REG or a SUBREG. If it's a subreg it may also contain a MEM before reload
> (because it is guaranteed to be reloaded into a register later). Anyway, the bottom line is that
> we have to be careful when taking REGNO of expressions during expand-time.
>
> This patch extracts the inner rtx in case we have a SUBREG and checks that it's a REG before
> checking its REGNO.
>
> Bootstrapped and tested on aarch64-none-linux-gnu. Tested aarch64-none-elf with RTL checking enabled
> (without this patch that doesn't build).
>
> Ok for trunk?
> Thanks,
> Kyrill
>
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/78362
>     * config/aarch64/aarch64.md (add<mode>3): Extract inner expression
>     from a subreg in operands[1] and don't call REGNO on a non-reg expression
>         when deciding to force operands[2] into a reg.
>
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/78362
>     * gcc.c-torture/compile/pr78362.c: New test.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register
  2016-11-23 14:17 ` Kyrill Tkachov
@ 2016-11-30 10:33   ` Kyrill Tkachov
  0 siblings, 0 replies; 5+ messages in thread
From: Kyrill Tkachov @ 2016-11-30 10:33 UTC (permalink / raw)
  To: GCC Patches; +Cc: Marcus Shawcroft, Richard Earnshaw, James Greenhalgh

Ping.

Thanks,
Kyrill

On 23/11/16 14:16, Kyrill Tkachov wrote:
>
> Ping.
> https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01664.html
>
> Thanks,
> Kyrill
>
> On 16/11/16 16:57, Kyrill Tkachov wrote:
>> Hi all,
>>
>> As the PR says we have an RTL checking failure that occurs when building libgcc for aarch64.
>> The expander code for addsi3 takes the REGNO of a SUBREG in operands[1]. The three operands
>> in the failing case are:
>> {(reg:SI 78), (subreg:SI (reg:DI 77) 0), (subreg:SI (reg:DI 73 [ ivtmp.9 ]) 0)}
>>
>> According to the documentation of register_operand (which is the predicate for operands[1]),
>> operands[1] can be a REG or a SUBREG. If it's a subreg it may also contain a MEM before reload
>> (because it is guaranteed to be reloaded into a register later). Anyway, the bottom line is that
>> we have to be careful when taking REGNO of expressions during expand-time.
>>
>> This patch extracts the inner rtx in case we have a SUBREG and checks that it's a REG before
>> checking its REGNO.
>>
>> Bootstrapped and tested on aarch64-none-linux-gnu. Tested aarch64-none-elf with RTL checking enabled
>> (without this patch that doesn't build).
>>
>> Ok for trunk?
>> Thanks,
>> Kyrill
>>
>> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     PR target/78362
>>     * config/aarch64/aarch64.md (add<mode>3): Extract inner expression
>>     from a subreg in operands[1] and don't call REGNO on a non-reg expression
>>         when deciding to force operands[2] into a reg.
>>
>> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     PR target/78362
>>     * gcc.c-torture/compile/pr78362.c: New test.
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register
  2016-11-16 16:57 [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register Kyrill Tkachov
  2016-11-23 14:17 ` Kyrill Tkachov
@ 2016-11-30 11:50 ` Ramana Radhakrishnan
  2016-11-30 11:55 ` James Greenhalgh
  2 siblings, 0 replies; 5+ messages in thread
From: Ramana Radhakrishnan @ 2016-11-30 11:50 UTC (permalink / raw)
  To: Kyrill Tkachov
  Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw, James Greenhalgh

On Wed, Nov 16, 2016 at 4:57 PM, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
> Hi all,
>
> As the PR says we have an RTL checking failure that occurs when building
> libgcc for aarch64.
> The expander code for addsi3 takes the REGNO of a SUBREG in operands[1]. The
> three operands
> in the failing case are:
> {(reg:SI 78), (subreg:SI (reg:DI 77) 0), (subreg:SI (reg:DI 73 [ ivtmp.9 ])
> 0)}
>
> According to the documentation of register_operand (which is the predicate
> for operands[1]),
> operands[1] can be a REG or a SUBREG. If it's a subreg it may also contain a
> MEM before reload
> (because it is guaranteed to be reloaded into a register later). Anyway, the
> bottom line is that
> we have to be careful when taking REGNO of expressions during expand-time.
>
> This patch extracts the inner rtx in case we have a SUBREG and checks that
> it's a REG before
> checking its REGNO.
>
> Bootstrapped and tested on aarch64-none-linux-gnu. Tested aarch64-none-elf
> with RTL checking enabled
> (without this patch that doesn't build).
>
> Ok for trunk?


LGTM but can't approve.

Ramana

> Thanks,
> Kyrill
>
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/78362
>     * config/aarch64/aarch64.md (add<mode>3): Extract inner expression
>     from a subreg in operands[1] and don't call REGNO on a non-reg
> expression
>         when deciding to force operands[2] into a reg.
>
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>     PR target/78362
>     * gcc.c-torture/compile/pr78362.c: New test.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register
  2016-11-16 16:57 [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register Kyrill Tkachov
  2016-11-23 14:17 ` Kyrill Tkachov
  2016-11-30 11:50 ` Ramana Radhakrishnan
@ 2016-11-30 11:55 ` James Greenhalgh
  2 siblings, 0 replies; 5+ messages in thread
From: James Greenhalgh @ 2016-11-30 11:55 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: GCC Patches, Marcus Shawcroft, Richard Earnshaw, nd

On Wed, Nov 16, 2016 at 04:57:29PM +0000, Kyrill Tkachov wrote:
> Hi all,
> 
> As the PR says we have an RTL checking failure that occurs when building
> libgcc for aarch64.  The expander code for addsi3 takes the REGNO of a SUBREG
> in operands[1]. The
> three operands in the failing case are:
> {(reg:SI 78), (subreg:SI (reg:DI 77) 0), (subreg:SI (reg:DI 73 [ ivtmp.9 ]) 0)}
> 
> According to the documentation of register_operand (which is the predicate
> for operands[1]), operands[1] can be a REG or a SUBREG. If it's a subreg it
> may also contain a MEM before reload (because it is guaranteed to be reloaded
> into a register later). Anyway, the bottom line is that we have to be careful
> when taking REGNO of expressions during expand-time.
> 
> This patch extracts the inner rtx in case we have a SUBREG and checks that
> it's a REG before checking its REGNO.
> 
> Bootstrapped and tested on aarch64-none-linux-gnu. Tested aarch64-none-elf
> with RTL checking enabled (without this patch that doesn't build).
> 
> Ok for trunk?

OK.

Thanks,
James

> Thanks,
> Kyrill
> 
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/78362
>     * config/aarch64/aarch64.md (add<mode>3): Extract inner expression
>     from a subreg in operands[1] and don't call REGNO on a non-reg expression
>         when deciding to force operands[2] into a reg.
> 
> 2016-11-16  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>     PR target/78362
>     * gcc.c-torture/compile/pr78362.c: New test.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-11-30 11:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-16 16:57 [PATCH][AArch64] PR target/78362: Make sure to only take REGNO of a register Kyrill Tkachov
2016-11-23 14:17 ` Kyrill Tkachov
2016-11-30 10:33   ` Kyrill Tkachov
2016-11-30 11:50 ` Ramana Radhakrishnan
2016-11-30 11:55 ` James Greenhalgh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).