diff mbox

Record equivalences for spill registers

Message ID 87inlfsr6e.fsf@linaro.org
State Accepted
Commit 8ffa3150d30b90a11aba7d7bba3c6462b6461101
Headers show

Commit Message

Richard Sandiford May 5, 2017, 7:23 a.m. UTC 
If we decide to allocate a call-clobbered register R to a value that
is live across a call, LRA will create a new spill register TMPR,
insert:

   TMPR <- R

before the call and

   R <- TMPR

after it.  But if we then failed to allocate a register to TMPR, we would
always spill it to the stack, even if R was known to be equivalent to
a constant or to some existing memory location.  And on AArch64, we'd
always fail to allocate such a register for 128-bit Advanced SIMD modes,
since no registers of those modes are call-preserved.

This patch avoids the problem by copying the equivalence information
from the original pseudo to the spill register.  It means that the
code for the testcase is as good with -O2 as it is with -O,
whereas previously the -O code was better.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


[Based on commit branches/ARM/sve-branch@247248]

2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	* lra-constraints.c (lra_copy_reg_equiv): New function.
	(split_reg): Use it to copy equivalence information from the
	original register to the spill register.

gcc/testsuite/
	* gcc.target/aarch64/spill_1.c: New test.

Comments

Jeff Law May 5, 2017, 4:50 p.m. UTC | #1 
On 05/05/2017 01:23 AM, Richard Sandiford wrote:
> If we decide to allocate a call-clobbered register R to a value that

> is live across a call, LRA will create a new spill register TMPR,

> insert:

> 

>     TMPR <- R

> 

> before the call and

> 

>     R <- TMPR

> 

> after it.  But if we then failed to allocate a register to TMPR, we would

> always spill it to the stack, even if R was known to be equivalent to

> a constant or to some existing memory location.  And on AArch64, we'd

> always fail to allocate such a register for 128-bit Advanced SIMD modes,

> since no registers of those modes are call-preserved.

> 

> This patch avoids the problem by copying the equivalence information

> from the original pseudo to the spill register.  It means that the

> code for the testcase is as good with -O2 as it is with -O,

> whereas previously the -O code was better.

> 

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

> 

> Thanks,

> Richard

> 

> 

> [Based on commit branches/ARM/sve-branch@247248]

> 

> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

> 

> gcc/

> 	* lra-constraints.c (lra_copy_reg_equiv): New function.

> 	(split_reg): Use it to copy equivalence information from the

> 	original register to the spill register.

> 

> gcc/testsuite/

> 	* gcc.target/aarch64/spill_1.c: New test.

OK.
jeff
Jim Wilson May 8, 2017, 4:30 a.m. UTC | #2 
On 05/05/2017 12:23 AM, Richard Sandiford wrote:
> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

> 	* lra-constraints.c (lra_copy_reg_equiv): New function.

> 	(split_reg): Use it to copy equivalence information from the

> 	original register to the spill register.


This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951

godump.o: In function `go_define(unsigned int, char const*)':
godump.c:(.text+0x36c): relocation truncated to fit:
R_AARCH64_ADR_PREL_LO21 against `.rodata'
godump.c:(.text+0x4b4): relocation truncated to fit: 
R_AARCH64_ADR_PREL_LO21 against `.rodata'

The godump.c.271r.ira file looks OK, I see

(insn 237 223 225 10 (set (reg/f:DI 489)
         (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49 
{*movdi_aarch64}
      (expr_list:REG_EQUIV (high:DI (label_ref 240))
         (insn_list:REG_LABEL_OPERAND 240 (nil))))
...
(insn 238 115 1157 10 (set (reg/f:DI 490)
         (lo_sum:DI (reg/f:DI 489)
             (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929 
{add_losym_di}
      (expr_list:REG_DEAD (reg/f:DI 489)
         (expr_list:REG_EQUIV (label_ref 240)
             (insn_list:REG_LABEL_OPERAND 240 (nil)))))

But in the godump.c.272r.reload file I see in a different basic block

(insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])
         (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49 
{*movdi_aarch64}
      (nil))

which is not OK.  This label ref is the address of a jumptable in the 
rodata section, and can't be loaded with a single instruction.  It looks 
like there needs to be some extra work when rematerializing, to handle 
equiv values that can't just be copied to a register.

I haven't had a chance to step through this in a debugger to see what is 
going on yet.

Jim
Andrew Pinski May 8, 2017, 5:26 a.m. UTC | #3 
On Sun, May 7, 2017 at 9:30 PM, Jim Wilson <jim.wilson@linaro.org> wrote:
> On 05/05/2017 12:23 AM, Richard Sandiford wrote:

>>

>> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

>>

>> gcc/

>>         * lra-constraints.c (lra_copy_reg_equiv): New function.

>>         (split_reg): Use it to copy equivalence information from the

>>         original register to the spill register.

>

>

> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951

>

> godump.o: In function `go_define(unsigned int, char const*)':

> godump.c:(.text+0x36c): relocation truncated to fit:

> R_AARCH64_ADR_PREL_LO21 against `.rodata'

> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21

> against `.rodata'

>

> The godump.c.271r.ira file looks OK, I see

>

> (insn 237 223 225 10 (set (reg/f:DI 489)

>         (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49

> {*movdi_aarch64}

>      (expr_list:REG_EQUIV (high:DI (label_ref 240))

>         (insn_list:REG_LABEL_OPERAND 240 (nil))))

> ...

> (insn 238 115 1157 10 (set (reg/f:DI 490)

>         (lo_sum:DI (reg/f:DI 489)

>             (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929

> {add_losym_di}

>      (expr_list:REG_DEAD (reg/f:DI 489)

>         (expr_list:REG_EQUIV (label_ref 240)

>             (insn_list:REG_LABEL_OPERAND 240 (nil)))))

>

> But in the godump.c.272r.reload file I see in a different basic block

>

> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])

>         (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49

> {*movdi_aarch64}

>      (nil))

>

> which is not OK.  This label ref is the address of a jumptable in the rodata

> section, and can't be loaded with a single instruction.  It looks like there

> needs to be some extra work when rematerializing, to handle equiv values

> that can't just be copied to a register.


Hmm, shouldn't have the mov rejected as being invalid?  I think that
is one issue with the aarch64 backend there.

See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html


I can't remember if the following patch was ever submitted or committed.

Here are my notes about this patch from the internal bug report we got
here at Cavium (back in 2013):

Switch tables are implemented using the tiny model but they are stored
in the rodata section which means they could overflow the address.

Note this patch most likely won't apply directly either:

From: Andrew Pinski <apinski@cavium.com>

Date: Thu, 15 Aug 2013 20:42:12 +0000 (-0700)
Subject: 2013-08-11  Andrew Pinski  <apinski@cavium.com>
X-Git-Tag: tc47_SDK_3_1_0_build_28~9
X-Git-Url: http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9

2013-08-11  Andrew Pinski  <apinski@cavium.com>

Bug #7079
* config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.
(*movdi_aarch64): Likewise.
(ldr_got_small_sidi): Add type attribute.
* config/aarch64/constraints.md (Ust): New constraint like S but only
if the symbol is SYMBOL_TINY_ABSOLUTE.
---

  "A memory address which uses a single base register with no offset."
  (and (match_code "mem")


>

> I haven't had a chance to step through this in a debugger to see what is

> going on yet.

>

> Jim

>diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64

index 3c8ff13..f4e1e91 100644
--- a/gcc/ChangeLog.aarch64
+++ b/gcc/ChangeLog.aarch64
@@ -1,3 +1,12 @@
+2013-08-11  Andrew Pinski  <apinski@cavium.com>
+
+ Bug #7079
+ * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.
+ (*movdi_aarch64): Likewise.
+ (ldr_got_small_sidi): Add type attribute.
+ * config/aarch64/constraints.md (Ust): New constraint like S but only
+ if the symbol is SYMBOL_TINY_ABSOLUTE.
+
 2013-08-14  Yufeng Zhang  <yufeng.zhang@arm.com>

  * function.c (assign_parm_find_data_types): Set passed_mode and
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 485bd59..0cd6a41 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -810,7 +810,7 @@

 (define_insn "*movsi_aarch64"
   [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,
m,r,r  ,*w, r,*w")
- (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,
m,rZ,*w,S,Ush,rZ,*w,*w"))]
+ (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,
m,rZ,*w,Ust,Ush,rZ,*w,*w"))]
   "(register_operand (operands[0], SImode)
     || aarch64_reg_or_zero (operands[1], SImode))"
   "@
@@ -836,7 +836,7 @@

 (define_insn "*movdi_aarch64"
   [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,
m,r,r,  *w, r,*w,w")
- (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,
m,rZ,*w,S,Ush,rZ,*w,*w,Dd"))]
+ (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,
m,rZ,*w,Ust,Ush,rZ,*w,*w,Dd"))]
   "(register_operand (operands[0], DImode)
     || aarch64_reg_or_zero (operands[1], DImode))"
   "@
@@ -3978,6 +3978,7 @@
   "TARGET_ILP32"
   "ldr\\t%w0, [%1, #:got_lo12:%a2]"
   [(set_attr "v8type" "load1")
+   (set_attr "type" "load1")
    (set_attr "mode" "DI")]
 )

diff --git a/gcc/config/aarch64/constraints.md
b/gcc/config/aarch64/constraints.md
index 2570400..a36bdaf 100644
--- a/gcc/config/aarch64/constraints.md
+++ b/gcc/config/aarch64/constraints.md
@@ -114,6 +114,12 @@
   (and (match_code "const_int")
        (match_test "(unsigned) exact_log2 (ival) <= 4")))

+(define_constraint "Ust"
+  "A constraint that matches an absolute symbolic address; only for
tiny model."
+  (and (match_code "const,symbol_ref,label_ref")
+       (match_test "aarch64_symbolic_address_p (op)
+    && aarch64_classify_symbolic_expression (op, SYMBOL_CONTEXT_ADR)
== SYMBOL_TINY_ABSOLUTE")))
+
 (define_memory_constraint "Q"
Andrew Pinski May 8, 2017, 5:28 a.m. UTC | #4 
On Sun, May 7, 2017 at 10:26 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Sun, May 7, 2017 at 9:30 PM, Jim Wilson <jim.wilson@linaro.org> wrote:

>> On 05/05/2017 12:23 AM, Richard Sandiford wrote:

>>>

>>> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

>>>

>>> gcc/

>>>         * lra-constraints.c (lra_copy_reg_equiv): New function.

>>>         (split_reg): Use it to copy equivalence information from the

>>>         original register to the spill register.

>>

>>

>> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951

>>

>> godump.o: In function `go_define(unsigned int, char const*)':

>> godump.c:(.text+0x36c): relocation truncated to fit:

>> R_AARCH64_ADR_PREL_LO21 against `.rodata'

>> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21

>> against `.rodata'

>>

>> The godump.c.271r.ira file looks OK, I see

>>

>> (insn 237 223 225 10 (set (reg/f:DI 489)

>>         (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49

>> {*movdi_aarch64}

>>      (expr_list:REG_EQUIV (high:DI (label_ref 240))

>>         (insn_list:REG_LABEL_OPERAND 240 (nil))))

>> ...

>> (insn 238 115 1157 10 (set (reg/f:DI 490)

>>         (lo_sum:DI (reg/f:DI 489)

>>             (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929

>> {add_losym_di}

>>      (expr_list:REG_DEAD (reg/f:DI 489)

>>         (expr_list:REG_EQUIV (label_ref 240)

>>             (insn_list:REG_LABEL_OPERAND 240 (nil)))))

>>

>> But in the godump.c.272r.reload file I see in a different basic block

>>

>> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])

>>         (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49

>> {*movdi_aarch64}

>>      (nil))

>>

>> which is not OK.  This label ref is the address of a jumptable in the rodata

>> section, and can't be loaded with a single instruction.  It looks like there

>> needs to be some extra work when rematerializing, to handle equiv values

>> that can't just be copied to a register.

>

> Hmm, shouldn't have the mov rejected as being invalid?  I think that

> is one issue with the aarch64 backend there.

>

> See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html

>

>

> I can't remember if the following patch was ever submitted or committed.

>

> Here are my notes about this patch from the internal bug report we got

> here at Cavium (back in 2013):

>

> Switch tables are implemented using the tiny model but they are stored

> in the rodata section which means they could overflow the address.



Some more notes:

(In reply to comment #15)
> (In reply to comment #14)

> > //(insn 793 35 795 (set (reg/f:DI 2 x2 [450])

> > //        (label_ref 456)) 32 {*movdi_aarch64}

> > //     (insn_list:REG_LABEL_OPERAND 456 (expr_list:REG_EQUAL (label_ref 456)

> > //            (nil))))

> > adr x2, .L806 // 793 *movdi_aarch64/9 [length = 4]

>

> Which is generated by the register allocator and we never split it into the

> adrp/add again.


The register allocator is doing an ok job as the backend said this is
a valid pattern.  We need a constraint for
*movdi_aarch64/*movsi_aarch64 which says this is not a valid pattern
and to go through the standard gen_movinsn path.


Thanks,
Andrew Pinski

>

> Note this patch most likely won't apply directly either:

>

> From: Andrew Pinski <apinski@cavium.com>

> Date: Thu, 15 Aug 2013 20:42:12 +0000 (-0700)

> Subject: 2013-08-11  Andrew Pinski  <apinski@cavium.com>

> X-Git-Tag: tc47_SDK_3_1_0_build_28~9

> X-Git-Url: http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9

>

> 2013-08-11  Andrew Pinski  <apinski@cavium.com>

>

> Bug #7079

> * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

> (*movdi_aarch64): Likewise.

> (ldr_got_small_sidi): Add type attribute.

> * config/aarch64/constraints.md (Ust): New constraint like S but only

> if the symbol is SYMBOL_TINY_ABSOLUTE.

> ---

>

> diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64

> index 3c8ff13..f4e1e91 100644

> --- a/gcc/ChangeLog.aarch64

> +++ b/gcc/ChangeLog.aarch64

> @@ -1,3 +1,12 @@

> +2013-08-11  Andrew Pinski  <apinski@cavium.com>

> +

> + Bug #7079

> + * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

> + (*movdi_aarch64): Likewise.

> + (ldr_got_small_sidi): Add type attribute.

> + * config/aarch64/constraints.md (Ust): New constraint like S but only

> + if the symbol is SYMBOL_TINY_ABSOLUTE.

> +

>  2013-08-14  Yufeng Zhang  <yufeng.zhang@arm.com>

>

>   * function.c (assign_parm_find_data_types): Set passed_mode and

> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md

> index 485bd59..0cd6a41 100644

> --- a/gcc/config/aarch64/aarch64.md

> +++ b/gcc/config/aarch64/aarch64.md

> @@ -810,7 +810,7 @@

>

>  (define_insn "*movsi_aarch64"

>    [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

> m,r,r  ,*w, r,*w")

> - (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

> m,rZ,*w,S,Ush,rZ,*w,*w"))]

> + (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

> m,rZ,*w,Ust,Ush,rZ,*w,*w"))]

>    "(register_operand (operands[0], SImode)

>      || aarch64_reg_or_zero (operands[1], SImode))"

>    "@

> @@ -836,7 +836,7 @@

>

>  (define_insn "*movdi_aarch64"

>    [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

> m,r,r,  *w, r,*w,w")

> - (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

> m,rZ,*w,S,Ush,rZ,*w,*w,Dd"))]

> + (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

> m,rZ,*w,Ust,Ush,rZ,*w,*w,Dd"))]

>    "(register_operand (operands[0], DImode)

>      || aarch64_reg_or_zero (operands[1], DImode))"

>    "@

> @@ -3978,6 +3978,7 @@

>    "TARGET_ILP32"

>    "ldr\\t%w0, [%1, #:got_lo12:%a2]"

>    [(set_attr "v8type" "load1")

> +   (set_attr "type" "load1")

>     (set_attr "mode" "DI")]

>  )

>

> diff --git a/gcc/config/aarch64/constraints.md

> b/gcc/config/aarch64/constraints.md

> index 2570400..a36bdaf 100644

> --- a/gcc/config/aarch64/constraints.md

> +++ b/gcc/config/aarch64/constraints.md

> @@ -114,6 +114,12 @@

>    (and (match_code "const_int")

>         (match_test "(unsigned) exact_log2 (ival) <= 4")))

>

> +(define_constraint "Ust"

> +  "A constraint that matches an absolute symbolic address; only for

> tiny model."

> +  (and (match_code "const,symbol_ref,label_ref")

> +       (match_test "aarch64_symbolic_address_p (op)

> +    && aarch64_classify_symbolic_expression (op, SYMBOL_CONTEXT_ADR)

> == SYMBOL_TINY_ABSOLUTE")))

> +

>  (define_memory_constraint "Q"

>   "A memory address which uses a single base register with no offset."

>   (and (match_code "mem")

>

>

>>

>> I haven't had a chance to step through this in a debugger to see what is

>> going on yet.

>>

>> Jim

>>
Richard Sandiford via Gcc-patches May 8, 2017, 6:37 a.m. UTC | #5 
Andrew Pinski <pinskia@gmail.com> writes:
> On Sun, May 7, 2017 at 9:30 PM, Jim Wilson <jim.wilson@linaro.org> wrote:

>> On 05/05/2017 12:23 AM, Richard Sandiford wrote:

>>>

>>> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

>>>

>>> gcc/

>>>         * lra-constraints.c (lra_copy_reg_equiv): New function.

>>>         (split_reg): Use it to copy equivalence information from the

>>>         original register to the spill register.

>>

>>

>> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951


Really sorry for the breakage.  I'd forgotten that this depended on:

  https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01550.html

although it looks like the exact failure I'd originally seen doesn't
trigger with current sources (at least, it didn't reproduce in my testing).

>> godump.o: In function `go_define(unsigned int, char const*)':

>> godump.c:(.text+0x36c): relocation truncated to fit:

>> R_AARCH64_ADR_PREL_LO21 against `.rodata'

>> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21

>> against `.rodata'

>>

>> The godump.c.271r.ira file looks OK, I see

>>

>> (insn 237 223 225 10 (set (reg/f:DI 489)

>>         (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49

>> {*movdi_aarch64}

>>      (expr_list:REG_EQUIV (high:DI (label_ref 240))

>>         (insn_list:REG_LABEL_OPERAND 240 (nil))))

>> ...

>> (insn 238 115 1157 10 (set (reg/f:DI 490)

>>         (lo_sum:DI (reg/f:DI 489)

>>             (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929

>> {add_losym_di}

>>      (expr_list:REG_DEAD (reg/f:DI 489)

>>         (expr_list:REG_EQUIV (label_ref 240)

>>             (insn_list:REG_LABEL_OPERAND 240 (nil)))))

>>

>> But in the godump.c.272r.reload file I see in a different basic block

>>

>> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])

>>         (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49

>> {*movdi_aarch64}

>>      (nil))

>>

>> which is not OK.  This label ref is the address of a jumptable in the rodata

>> section, and can't be loaded with a single instruction.  It looks like there

>> needs to be some extra work when rematerializing, to handle equiv values

>> that can't just be copied to a register.

>

> Hmm, shouldn't have the mov rejected as being invalid?  I think that

> is one issue with the aarch64 backend there.

>

> See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html

>

>

> I can't remember if the following patch was ever submitted or committed.

>

> Here are my notes about this patch from the internal bug report we got

> here at Cavium (back in 2013):

>

> Switch tables are implemented using the tiny model but they are stored

> in the rodata section which means they could overflow the address.

>

> Note this patch most likely won't apply directly either:

>

> From: Andrew Pinski <apinski@cavium.com>

> Date: Thu, 15 Aug 2013 20:42:12 +0000 (-0700)

> Subject: 2013-08-11  Andrew Pinski  <apinski@cavium.com>

> X-Git-Tag: tc47_SDK_3_1_0_build_28~9

> X-Git-Url: http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9

>

> 2013-08-11  Andrew Pinski  <apinski@cavium.com>

>

> Bug #7079

> * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

> (*movdi_aarch64): Likewise.

> (ldr_got_small_sidi): Add type attribute.

> * config/aarch64/constraints.md (Ust): New constraint like S but only

> if the symbol is SYMBOL_TINY_ABSOLUTE.

> ---

>

> diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64

> index 3c8ff13..f4e1e91 100644

> --- a/gcc/ChangeLog.aarch64

> +++ b/gcc/ChangeLog.aarch64

> @@ -1,3 +1,12 @@

> +2013-08-11  Andrew Pinski  <apinski@cavium.com>

> +

> + Bug #7079

> + * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

> + (*movdi_aarch64): Likewise.

> + (ldr_got_small_sidi): Add type attribute.

> + * config/aarch64/constraints.md (Ust): New constraint like S but only

> + if the symbol is SYMBOL_TINY_ABSOLUTE.

> +

>  2013-08-14  Yufeng Zhang  <yufeng.zhang@arm.com>

>

>   * function.c (assign_parm_find_data_types): Set passed_mode and

> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md

> index 485bd59..0cd6a41 100644

> --- a/gcc/config/aarch64/aarch64.md

> +++ b/gcc/config/aarch64/aarch64.md

> @@ -810,7 +810,7 @@

>

>  (define_insn "*movsi_aarch64"

>    [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

> m,r,r  ,*w, r,*w")

> - (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

> m,rZ,*w,S,Ush,rZ,*w,*w"))]

> + (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

> m,rZ,*w,Ust,Ush,rZ,*w,*w"))]

>    "(register_operand (operands[0], SImode)

>      || aarch64_reg_or_zero (operands[1], SImode))"

>    "@

> @@ -836,7 +836,7 @@

>

>  (define_insn "*movdi_aarch64"

>    [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

> m,r,r,  *w, r,*w,w")

> - (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

> m,rZ,*w,S,Ush,rZ,*w,*w,Dd"))]

> + (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

> m,rZ,*w,Ust,Ush,rZ,*w,*w,Dd"))]

>    "(register_operand (operands[0], DImode)

>      || aarch64_reg_or_zero (operands[1], DImode))"

>    "@

> @@ -3978,6 +3978,7 @@

>    "TARGET_ILP32"

>    "ldr\\t%w0, [%1, #:got_lo12:%a2]"

>    [(set_attr "v8type" "load1")

> +   (set_attr "type" "load1")

>     (set_attr "mode" "DI")]

>  )

>

> diff --git a/gcc/config/aarch64/constraints.md

> b/gcc/config/aarch64/constraints.md

> index 2570400..a36bdaf 100644

> --- a/gcc/config/aarch64/constraints.md

> +++ b/gcc/config/aarch64/constraints.md

> @@ -114,6 +114,12 @@

>    (and (match_code "const_int")

>         (match_test "(unsigned) exact_log2 (ival) <= 4")))

>

> +(define_constraint "Ust"

> +  "A constraint that matches an absolute symbolic address; only for

> tiny model."

> +  (and (match_code "const,symbol_ref,label_ref")

> +       (match_test "aarch64_symbolic_address_p (op)

> +    && aarch64_classify_symbolic_expression (op, SYMBOL_CONTEXT_ADR)

> == SYMBOL_TINY_ABSOLUTE")))

> +

>  (define_memory_constraint "Q"

>   "A memory address which uses a single base register with no offset."

>   (and (match_code "mem")


Looks like this is functionally equivalent to my patch, although it
obviously predates it by quite some way.

Thanks,
Richard
Richard Sandiford via Gcc-patches May 8, 2017, 6:47 a.m. UTC | #6 
On Sun, May 7, 2017 at 11:37 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Andrew Pinski <pinskia@gmail.com> writes:

>> On Sun, May 7, 2017 at 9:30 PM, Jim Wilson <jim.wilson@linaro.org> wrote:

>>> On 05/05/2017 12:23 AM, Richard Sandiford wrote:

>>>>

>>>> 2017-05-05  Richard Sandiford  <richard.sandiford@linaro.org>

>>>>

>>>> gcc/

>>>>         * lra-constraints.c (lra_copy_reg_equiv): New function.

>>>>         (split_reg): Use it to copy equivalence information from the

>>>>         original register to the spill register.

>>>

>>>

>>> This patch breaks aarch64 bootstrap.  I get a link error for lto1 and f951

>

> Really sorry for the breakage.  I'd forgotten that this depended on:

>

>   https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01550.html

>

> although it looks like the exact failure I'd originally seen doesn't

> trigger with current sources (at least, it didn't reproduce in my testing).

>

>>> godump.o: In function `go_define(unsigned int, char const*)':

>>> godump.c:(.text+0x36c): relocation truncated to fit:

>>> R_AARCH64_ADR_PREL_LO21 against `.rodata'

>>> godump.c:(.text+0x4b4): relocation truncated to fit: R_AARCH64_ADR_PREL_LO21

>>> against `.rodata'

>>>

>>> The godump.c.271r.ira file looks OK, I see

>>>

>>> (insn 237 223 225 10 (set (reg/f:DI 489)

>>>         (high:DI (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 49

>>> {*movdi_aarch64}

>>>      (expr_list:REG_EQUIV (high:DI (label_ref 240))

>>>         (insn_list:REG_LABEL_OPERAND 240 (nil))))

>>> ...

>>> (insn 238 115 1157 10 (set (reg/f:DI 490)

>>>         (lo_sum:DI (reg/f:DI 489)

>>>             (label_ref 240))) "../../gcc-svn/gcc/godump.c":174 929

>>> {add_losym_di}

>>>      (expr_list:REG_DEAD (reg/f:DI 489)

>>>         (expr_list:REG_EQUIV (label_ref 240)

>>>             (insn_list:REG_LABEL_OPERAND 240 (nil)))))

>>>

>>> But in the godump.c.272r.reload file I see in a different basic block

>>>

>>> (insn 1244 76 1161 22 (set (reg/f:DI 7 x7 [490])

>>>         (label_ref 240)) "../../gcc-svn/gcc/godump.c":221 49

>>> {*movdi_aarch64}

>>>      (nil))

>>>

>>> which is not OK.  This label ref is the address of a jumptable in the rodata

>>> section, and can't be loaded with a single instruction.  It looks like there

>>> needs to be some extra work when rematerializing, to handle equiv values

>>> that can't just be copied to a register.

>>

>> Hmm, shouldn't have the mov rejected as being invalid?  I think that

>> is one issue with the aarch64 backend there.

>>

>> See https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01800.html

>>

>>

>> I can't remember if the following patch was ever submitted or committed.

>>

>> Here are my notes about this patch from the internal bug report we got

>> here at Cavium (back in 2013):

>>

>> Switch tables are implemented using the tiny model but they are stored

>> in the rodata section which means they could overflow the address.

>>

>> Note this patch most likely won't apply directly either:

>>

>> From: Andrew Pinski <apinski@cavium.com>

>> Date: Thu, 15 Aug 2013 20:42:12 +0000 (-0700)

>> Subject: 2013-08-11  Andrew Pinski  <apinski@cavium.com>

>> X-Git-Tag: tc47_SDK_3_1_0_build_28~9

>> X-Git-Url: http://cagit1.caveonetworks.com/git/?p=toolchain%2Fgcc.git;a=commitdiff_plain;h=1714f167b575956801264db5cd2300203a58e3e9

>>

>> 2013-08-11  Andrew Pinski  <apinski@cavium.com>

>>

>> Bug #7079

>> * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

>> (*movdi_aarch64): Likewise.

>> (ldr_got_small_sidi): Add type attribute.

>> * config/aarch64/constraints.md (Ust): New constraint like S but only

>> if the symbol is SYMBOL_TINY_ABSOLUTE.

>> ---

>>

>> diff --git a/gcc/ChangeLog.aarch64 b/gcc/ChangeLog.aarch64

>> index 3c8ff13..f4e1e91 100644

>> --- a/gcc/ChangeLog.aarch64

>> +++ b/gcc/ChangeLog.aarch64

>> @@ -1,3 +1,12 @@

>> +2013-08-11  Andrew Pinski  <apinski@cavium.com>

>> +

>> + Bug #7079

>> + * config/aarch64/aarch64.md (*movsi_aarch64): Use Ust instead of S constrain.

>> + (*movdi_aarch64): Likewise.

>> + (ldr_got_small_sidi): Add type attribute.

>> + * config/aarch64/constraints.md (Ust): New constraint like S but only

>> + if the symbol is SYMBOL_TINY_ABSOLUTE.

>> +

>>  2013-08-14  Yufeng Zhang  <yufeng.zhang@arm.com>

>>

>>   * function.c (assign_parm_find_data_types): Set passed_mode and

>> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md

>> index 485bd59..0cd6a41 100644

>> --- a/gcc/config/aarch64/aarch64.md

>> +++ b/gcc/config/aarch64/aarch64.md

>> @@ -810,7 +810,7 @@

>>

>>  (define_insn "*movsi_aarch64"

>>    [(set (match_operand:SI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

>> m,r,r  ,*w, r,*w")

>> - (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

>> m,rZ,*w,S,Ush,rZ,*w,*w"))]

>> + (match_operand:SI 1 "aarch64_mov_operand"  " r,r,k,M,m,

>> m,rZ,*w,Ust,Ush,rZ,*w,*w"))]

>>    "(register_operand (operands[0], SImode)

>>      || aarch64_reg_or_zero (operands[1], SImode))"

>>    "@

>> @@ -836,7 +836,7 @@

>>

>>  (define_insn "*movdi_aarch64"

>>    [(set (match_operand:DI 0 "nonimmediate_operand" "=r,k,r,r,r,*w,m,

>> m,r,r,  *w, r,*w,w")

>> - (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

>> m,rZ,*w,S,Ush,rZ,*w,*w,Dd"))]

>> + (match_operand:DI 1 "aarch64_mov_operand"  " r,r,k,N,m,

>> m,rZ,*w,Ust,Ush,rZ,*w,*w,Dd"))]

>>    "(register_operand (operands[0], DImode)

>>      || aarch64_reg_or_zero (operands[1], DImode))"

>>    "@

>> @@ -3978,6 +3978,7 @@

>>    "TARGET_ILP32"

>>    "ldr\\t%w0, [%1, #:got_lo12:%a2]"

>>    [(set_attr "v8type" "load1")

>> +   (set_attr "type" "load1")

>>     (set_attr "mode" "DI")]

>>  )

>>

>> diff --git a/gcc/config/aarch64/constraints.md

>> b/gcc/config/aarch64/constraints.md

>> index 2570400..a36bdaf 100644

>> --- a/gcc/config/aarch64/constraints.md

>> +++ b/gcc/config/aarch64/constraints.md

>> @@ -114,6 +114,12 @@

>>    (and (match_code "const_int")

>>         (match_test "(unsigned) exact_log2 (ival) <= 4")))

>>

>> +(define_constraint "Ust"

>> +  "A constraint that matches an absolute symbolic address; only for

>> tiny model."

>> +  (and (match_code "const,symbol_ref,label_ref")

>> +       (match_test "aarch64_symbolic_address_p (op)

>> +    && aarch64_classify_symbolic_expression (op, SYMBOL_CONTEXT_ADR)

>> == SYMBOL_TINY_ABSOLUTE")))

>> +

>>  (define_memory_constraint "Q"

>>   "A memory address which uses a single base register with no offset."

>>   (and (match_code "mem")

>

> Looks like this is functionally equivalent to my patch, although it

> obviously predates it by quite some way.


I did not even notice you had a similar patch outstanding until you
pointed it out.  But I think it is obvious as the current code is
obviously wrong and we both came up with the same solution (3/4 years
apart).

Thanks,
Andrew

>

> Thanks,

> Richard
Jim Wilson May 9, 2017, 9:19 p.m. UTC | #7 
On Sun, May 7, 2017 at 11:47 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Sun, May 7, 2017 at 11:37 PM, Richard Sandiford

> <richard.sandiford@linaro.org> wrote:

>> Really sorry for the breakage.  I'd forgotten that this depended on:

>>

>>   https://gcc.gnu.org/ml/gcc-patches/2017-03/msg01550.html


Thanks, this does fix my build failures.  And I see that it is already
approved and checked in, so that's good.

Jim
diff mbox

Patch

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	2017-04-18 19:52:35.062175087 +0100
+++ gcc/lra-constraints.c	2017-05-05 08:19:18.243479648 +0100
@@ -5394,6 +5394,29 @@  choose_split_class (enum reg_class alloc
 #endif
 }
 
+/* Copy any equivalence information from ORIGINAL_REGNO to NEW_REGNO.
+   It only makes sense to call this function if NEW_REGNO is always
+   equal to ORIGINAL_REGNO.  */
+
+static void
+lra_copy_reg_equiv (unsigned int new_regno, unsigned int original_regno)
+{
+  if (!ira_reg_equiv[original_regno].defined_p)
+    return;
+
+  ira_expand_reg_equiv ();
+  ira_reg_equiv[new_regno].defined_p = true;
+  if (ira_reg_equiv[original_regno].memory)
+    ira_reg_equiv[new_regno].memory
+      = copy_rtx (ira_reg_equiv[original_regno].memory);
+  if (ira_reg_equiv[original_regno].constant)
+    ira_reg_equiv[new_regno].constant
+      = copy_rtx (ira_reg_equiv[original_regno].constant);
+  if (ira_reg_equiv[original_regno].invariant)
+    ira_reg_equiv[new_regno].invariant
+      = copy_rtx (ira_reg_equiv[original_regno].invariant);
+}
+
 /* Do split transformations for insn INSN, which defines or uses
    ORIGINAL_REGNO.  NEXT_USAGE_INSNS specifies which instruction in
    the EBB next uses ORIGINAL_REGNO; it has the same form as the
@@ -5515,6 +5538,7 @@  split_reg (bool before_p, int original_r
       new_reg = lra_create_new_reg (mode, original_reg, rclass, "split");
       reg_renumber[REGNO (new_reg)] = hard_regno;
     }
+  int new_regno = REGNO (new_reg);
   save = emit_spill_move (true, new_reg, original_reg);
   if (NEXT_INSN (save) != NULL_RTX && !call_save_p)
     {
@@ -5523,7 +5547,7 @@  split_reg (bool before_p, int original_r
 	  fprintf
 	    (lra_dump_file,
 	     "	  Rejecting split %d->%d resulting in > 2 save insns:\n",
-	     original_regno, REGNO (new_reg));
+	     original_regno, new_regno);
 	  dump_rtl_slim (lra_dump_file, save, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
@@ -5538,18 +5562,24 @@  split_reg (bool before_p, int original_r
 	  fprintf (lra_dump_file,
 		   "	Rejecting split %d->%d "
 		   "resulting in > 2 restore insns:\n",
-		   original_regno, REGNO (new_reg));
+		   original_regno, new_regno);
 	  dump_rtl_slim (lra_dump_file, restore, NULL, -1, 0);
 	  fprintf (lra_dump_file,
 		   "	))))))))))))))))))))))))))))))))))))))))))))))))\n");
 	}
       return false;
     }
+  /* Transfer equivalence information to the spill register, so that
+     if we fail to allocate the spill register, we have the option of
+     rematerializing the original value instead of spilling to the stack.  */
+  if (!HARD_REGISTER_NUM_P (original_regno)
+      && mode == PSEUDO_REGNO_MODE (original_regno))
+    lra_copy_reg_equiv (new_regno, original_regno);
   after_p = usage_insns[original_regno].after_p;
-  lra_reg_info[REGNO (new_reg)].restore_rtx = regno_reg_rtx[original_regno];
-  bitmap_set_bit (&check_only_regs, REGNO (new_reg));
+  lra_reg_info[new_regno].restore_rtx = regno_reg_rtx[original_regno];
+  bitmap_set_bit (&check_only_regs, new_regno);
   bitmap_set_bit (&check_only_regs, original_regno);
-  bitmap_set_bit (&lra_split_regs, REGNO (new_reg));
+  bitmap_set_bit (&lra_split_regs, new_regno);
   for (;;)
     {
       if (GET_CODE (next_usage_insns) != INSN_LIST)
@@ -5565,7 +5595,7 @@  split_reg (bool before_p, int original_r
       if (lra_dump_file != NULL)
 	{
 	  fprintf (lra_dump_file, "    Split reuse change %d->%d:\n",
-		   original_regno, REGNO (new_reg));
+		   original_regno, new_regno);
 	  dump_insn_slim (lra_dump_file, as_a <rtx_insn *> (usage_insn));
 	}
     }
Index: gcc/testsuite/gcc.target/aarch64/spill_1.c
===================================================================
--- /dev/null	2017-05-05 07:30:08.141451281 +0100
+++ gcc/testsuite/gcc.target/aarch64/spill_1.c	2017-05-05 08:19:18.244456716 +0100
@@ -0,0 +1,18 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+typedef int v4si __attribute__ ((vector_size (16)));
+
+void bar (void);
+void
+foo (void)
+{
+  v4si x = { 1, 1, 1, 1 };
+  asm ("# %0" :: "w" (x));
+  bar ();
+  asm ("# %0" :: "w" (x));
+}
+
+/* { dg-final { scan-assembler-times {\tmovi\tv[0-9]+\.4s,} 2 } } */
+/* { dg-final { scan-assembler-not {\tldr\t} } } */
+/* { dg-final { scan-assembler-not {\tstr\t} } } */