| Message ID | CA+=Sn1kixX=UF9ZyyrAhHP4J+6rqyABY4kMxCaWqsbPErWhFsA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 --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"