| Message ID | 1386105892-2379-6-git-send-email-apinski@cavium.com |
|---|---|
| State | New |
| Headers | show |
On 12/03/13 21:24, Andrew Pinski wrote: > Hi, > With ILP32, some simple usage of TLS variables causes an unrecognizable > instruction due to needing to use SImode for loading pointers from memory. > This fixes the three (tlsie_small, tlsle_small, tlsdesc_small) patterns to > support SImode for pointers. > > OK? Build and tested on aarch64-elf with no regressions. > > Thanks, > Andrew Pinski > > * config/aarch64/aarch64.c (aarch64_load_symref_appropriately): > Handle TLS for ILP32. > * config/aarch64/aarch64.md (tlsie_small): Change to an expand to > handle ILP32. > (tlsie_small_<mode>): New pattern. > (tlsle_small): Change to an expand to handle ILP32. > (tlsle_small_<mode>): New pattern. > (tlsdesc_small): Change to an expand to handle ILP32. > (tlsdesc_small_<mode>): New pattern. > --- > gcc/ChangeLog | 12 ++++++ > gcc/config/aarch64/aarch64.c | 23 ++++++++++-- > gcc/config/aarch64/aarch64.md | 76 ++++++++++++++++++++++++++++++++++------- > 3 files changed, 94 insertions(+), 17 deletions(-) > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index b1b4eef..a3e4532 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -628,22 +628,37 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm, > > case SYMBOL_SMALL_TLSDESC: > { > - rtx x0 = gen_rtx_REG (Pmode, R0_REGNUM); > + enum machine_mode mode = GET_MODE (dest); > + rtx x0 = gen_rtx_REG (mode, R0_REGNUM); > rtx tp; > > + gcc_assert (mode == Pmode || mode == ptr_mode); > + > emit_insn (gen_tlsdesc_small (imm)); > tp = aarch64_load_tp (NULL); > - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, x0))); > + > + if (mode != Pmode) > + tp = gen_lowpart (mode, tp); > + > + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, x0))); > set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); > return; > } > > case SYMBOL_SMALL_GOTTPREL: > { > - rtx tmp_reg = gen_reg_rtx (Pmode); > + enum machine_mode mode = GET_MODE (dest); > + rtx tmp_reg = gen_reg_rtx (mode); > rtx tp = aarch64_load_tp (NULL); > + > + gcc_assert (mode == Pmode || mode == ptr_mode); > + > emit_insn (gen_tlsie_small (tmp_reg, imm)); > - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, tmp_reg))); > + > + if (mode != Pmode) > + tp = gen_lowpart (mode, tp); > + > + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, tmp_reg))); > set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); > return; > } > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 313517f..08fcc94 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -3577,35 +3577,85 @@ > [(set_attr "type" "call") > (set_attr "length" "16")]) > > -(define_insn "tlsie_small" > - [(set (match_operand:DI 0 "register_operand" "=r") > - (unspec:DI [(match_operand:DI 1 "aarch64_tls_ie_symref" "S")] > +(define_expand "tlsie_small" > + [(set (match_operand 0 "register_operand" "=r") > + (unspec [(match_operand 1 "aarch64_tls_ie_symref" "S")] > + UNSPEC_GOTSMALLTLS))] > + "" > +{ > + if (TARGET_ILP32) > + { > + operands[0] = gen_lowpart (ptr_mode, operands[0]); > + emit_insn (gen_tlsie_small_si (operands[0], operands[1])); > + } > + else > + emit_insn (gen_tlsie_small_di (operands[0], operands[1])); > + DONE; > +}) > + > +(define_insn "tlsie_small_<mode>" > + [(set (match_operand:PTR 0 "register_operand" "=r") > + (unspec:PTR [(match_operand 1 "aarch64_tls_ie_symref" "S")] > UNSPEC_GOTSMALLTLS))] > "" > - "adrp\\t%0, %A1\;ldr\\t%0, [%0, #%L1]" > + "adrp\\t%0, %A1\;ldr\\t%<w>0, [%0, #%L1]" > [(set_attr "type" "load1") > (set_attr "length" "8")] > ) > > -(define_insn "tlsle_small" > - [(set (match_operand:DI 0 "register_operand" "=r") > - (unspec:DI [(match_operand:DI 1 "register_operand" "r") > - (match_operand:DI 2 "aarch64_tls_le_symref" "S")] > + > +(define_expand "tlsle_small" > + [(set (match_operand 0 "register_operand" "=r") > + (unspec [(match_operand 1 "register_operand" "r") > + (match_operand 2 "aarch64_tls_le_symref" "S")] > + UNSPEC_GOTSMALLTLS))] > + "" > +{ > + if (TARGET_ILP32) > + { > + rtx temp = gen_reg_rtx (ptr_mode); > + operands[1] = gen_lowpart (ptr_mode, operands[1]); > + emit_insn (gen_tlsle_small_si (temp, operands[1], operands[2])); > + emit_move_insn (operands[0], gen_lowpart (GET_MODE (operands[0]), temp)); > + } Looks like you hit the similar issue where the matched RTX can have either SImode or DImode in ILP32. The mechanism looks OK but I think the approach that 'add_losym' adopts is neater, which checks on the mode instead of TARGET_ILP32 and calls gen_add_losym_di or gen_add_losym_si accordingly. Note that the iterator used in add_losym_<mode> is P instead of PTR. Same for tlsie_small above. > + else > + emit_insn (gen_tlsle_small_di (operands[0], operands[1], operands[2])); > + DONE; Brackets for the else body to align with the if body; same for tlsie_small above. Yufeng
On Wed, Dec 4, 2013 at 10:12 AM, Yufeng Zhang <Yufeng.Zhang@arm.com> wrote: > On 12/03/13 21:24, Andrew Pinski wrote: >> >> Hi, >> With ILP32, some simple usage of TLS variables causes an unrecognizable >> instruction due to needing to use SImode for loading pointers from memory. >> This fixes the three (tlsie_small, tlsle_small, tlsdesc_small) patterns to >> support SImode for pointers. >> >> OK? Build and tested on aarch64-elf with no regressions. >> >> Thanks, >> Andrew Pinski >> >> * config/aarch64/aarch64.c (aarch64_load_symref_appropriately): >> Handle TLS for ILP32. >> * config/aarch64/aarch64.md (tlsie_small): Change to an expand to >> handle ILP32. >> (tlsie_small_<mode>): New pattern. >> (tlsle_small): Change to an expand to handle ILP32. >> (tlsle_small_<mode>): New pattern. >> (tlsdesc_small): Change to an expand to handle ILP32. >> (tlsdesc_small_<mode>): New pattern. >> --- >> gcc/ChangeLog | 12 ++++++ >> gcc/config/aarch64/aarch64.c | 23 ++++++++++-- >> gcc/config/aarch64/aarch64.md | 76 >> ++++++++++++++++++++++++++++++++++------- >> 3 files changed, 94 insertions(+), 17 deletions(-) >> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >> index b1b4eef..a3e4532 100644 >> --- a/gcc/config/aarch64/aarch64.c >> +++ b/gcc/config/aarch64/aarch64.c >> @@ -628,22 +628,37 @@ aarch64_load_symref_appropriately (rtx dest, rtx >> imm, >> >> case SYMBOL_SMALL_TLSDESC: >> { >> - rtx x0 = gen_rtx_REG (Pmode, R0_REGNUM); >> + enum machine_mode mode = GET_MODE (dest); >> + rtx x0 = gen_rtx_REG (mode, R0_REGNUM); >> rtx tp; >> >> + gcc_assert (mode == Pmode || mode == ptr_mode); >> + >> emit_insn (gen_tlsdesc_small (imm)); >> tp = aarch64_load_tp (NULL); >> - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, >> x0))); >> + >> + if (mode != Pmode) >> + tp = gen_lowpart (mode, tp); >> + >> + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, x0))); >> set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); >> return; >> } >> >> case SYMBOL_SMALL_GOTTPREL: >> { >> - rtx tmp_reg = gen_reg_rtx (Pmode); >> + enum machine_mode mode = GET_MODE (dest); >> + rtx tmp_reg = gen_reg_rtx (mode); >> rtx tp = aarch64_load_tp (NULL); >> + >> + gcc_assert (mode == Pmode || mode == ptr_mode); >> + >> emit_insn (gen_tlsie_small (tmp_reg, imm)); >> - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, >> tmp_reg))); >> + >> + if (mode != Pmode) >> + tp = gen_lowpart (mode, tp); >> + >> + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, >> tmp_reg))); >> set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); >> return; >> } >> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >> index 313517f..08fcc94 100644 >> --- a/gcc/config/aarch64/aarch64.md >> +++ b/gcc/config/aarch64/aarch64.md >> @@ -3577,35 +3577,85 @@ >> [(set_attr "type" "call") >> (set_attr "length" "16")]) >> >> -(define_insn "tlsie_small" >> - [(set (match_operand:DI 0 "register_operand" "=r") >> - (unspec:DI [(match_operand:DI 1 "aarch64_tls_ie_symref" "S")] >> +(define_expand "tlsie_small" >> + [(set (match_operand 0 "register_operand" "=r") >> + (unspec [(match_operand 1 "aarch64_tls_ie_symref" "S")] >> + UNSPEC_GOTSMALLTLS))] >> + "" >> +{ >> + if (TARGET_ILP32) >> + { >> + operands[0] = gen_lowpart (ptr_mode, operands[0]); >> + emit_insn (gen_tlsie_small_si (operands[0], operands[1])); >> + } >> + else >> + emit_insn (gen_tlsie_small_di (operands[0], operands[1])); >> + DONE; >> +}) >> + >> +(define_insn "tlsie_small_<mode>" >> + [(set (match_operand:PTR 0 "register_operand" "=r") >> + (unspec:PTR [(match_operand 1 "aarch64_tls_ie_symref" "S")] >> UNSPEC_GOTSMALLTLS))] >> "" >> - "adrp\\t%0, %A1\;ldr\\t%0, [%0, #%L1]" >> + "adrp\\t%0, %A1\;ldr\\t%<w>0, [%0, #%L1]" >> [(set_attr "type" "load1") >> (set_attr "length" "8")] >> ) >> >> -(define_insn "tlsle_small" >> - [(set (match_operand:DI 0 "register_operand" "=r") >> - (unspec:DI [(match_operand:DI 1 "register_operand" "r") >> - (match_operand:DI 2 "aarch64_tls_le_symref" "S")] >> + >> +(define_expand "tlsle_small" >> + [(set (match_operand 0 "register_operand" "=r") >> + (unspec [(match_operand 1 "register_operand" "r") >> + (match_operand 2 "aarch64_tls_le_symref" "S")] >> + UNSPEC_GOTSMALLTLS))] >> + "" >> +{ >> + if (TARGET_ILP32) >> + { >> + rtx temp = gen_reg_rtx (ptr_mode); >> + operands[1] = gen_lowpart (ptr_mode, operands[1]); >> + emit_insn (gen_tlsle_small_si (temp, operands[1], operands[2])); >> + emit_move_insn (operands[0], gen_lowpart (GET_MODE (operands[0]), >> temp)); >> + } > > > Looks like you hit the similar issue where the matched RTX can have either > SImode or DImode in ILP32. The mechanism looks OK but I think the approach > that 'add_losym' adopts is neater, which checks on the mode instead of > TARGET_ILP32 and calls gen_add_losym_di or gen_add_losym_si accordingly. > Note that the iterator used in add_losym_<mode> is P instead of PTR. Yes I agree with this and will fix this. > > Same for tlsie_small above. But not with this one, tlsie_small should rather be similar to ldr_got_small instead. > > >> + else >> + emit_insn (gen_tlsle_small_di (operands[0], operands[1], >> operands[2])); >> + DONE; > > > Brackets for the else body to align with the if body; same for tlsie_small > above. There is no requirement in the coding style for this. In fact there are examples of the coding style I used here all over GCC; even in aarch64.md (rotl<mode>3) and aarch64.c (aarch64_function_arg_alignment). > > > Yufeng >
On 02/25/14 01:23, Andrew Pinski wrote: > On Wed, Dec 4, 2013 at 10:12 AM, Yufeng Zhang<Yufeng.Zhang@arm.com> wrote: >> On 12/03/13 21:24, Andrew Pinski wrote: [snip] >>> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md >>> index 313517f..08fcc94 100644 >>> --- a/gcc/config/aarch64/aarch64.md >>> +++ b/gcc/config/aarch64/aarch64.md >>> @@ -3577,35 +3577,85 @@ >>> [(set_attr "type" "call") >>> (set_attr "length" "16")]) >>> >>> -(define_insn "tlsie_small" >>> - [(set (match_operand:DI 0 "register_operand" "=r") >>> - (unspec:DI [(match_operand:DI 1 "aarch64_tls_ie_symref" "S")] >>> +(define_expand "tlsie_small" >>> + [(set (match_operand 0 "register_operand" "=r") >>> + (unspec [(match_operand 1 "aarch64_tls_ie_symref" "S")] >>> + UNSPEC_GOTSMALLTLS))] >>> + "" >>> +{ >>> + if (TARGET_ILP32) >>> + { >>> + operands[0] = gen_lowpart (ptr_mode, operands[0]); >>> + emit_insn (gen_tlsie_small_si (operands[0], operands[1])); >>> + } >>> + else >>> + emit_insn (gen_tlsie_small_di (operands[0], operands[1])); >>> + DONE; >>> +}) >>> + >>> +(define_insn "tlsie_small_<mode>" >>> + [(set (match_operand:PTR 0 "register_operand" "=r") >>> + (unspec:PTR [(match_operand 1 "aarch64_tls_ie_symref" "S")] >>> UNSPEC_GOTSMALLTLS))] >>> "" >>> - "adrp\\t%0, %A1\;ldr\\t%0, [%0, #%L1]" >>> + "adrp\\t%0, %A1\;ldr\\t%<w>0, [%0, #%L1]" >>> [(set_attr "type" "load1") >>> (set_attr "length" "8")] >>> ) >>> >>> -(define_insn "tlsle_small" >>> - [(set (match_operand:DI 0 "register_operand" "=r") >>> - (unspec:DI [(match_operand:DI 1 "register_operand" "r") >>> - (match_operand:DI 2 "aarch64_tls_le_symref" "S")] >>> + >>> +(define_expand "tlsle_small" >>> + [(set (match_operand 0 "register_operand" "=r") >>> + (unspec [(match_operand 1 "register_operand" "r") >>> + (match_operand 2 "aarch64_tls_le_symref" "S")] >>> + UNSPEC_GOTSMALLTLS))] >>> + "" >>> +{ >>> + if (TARGET_ILP32) >>> + { >>> + rtx temp = gen_reg_rtx (ptr_mode); >>> + operands[1] = gen_lowpart (ptr_mode, operands[1]); >>> + emit_insn (gen_tlsle_small_si (temp, operands[1], operands[2])); >>> + emit_move_insn (operands[0], gen_lowpart (GET_MODE (operands[0]), >>> temp)); >>> + } >> >> >> Looks like you hit the similar issue where the matched RTX can have either >> SImode or DImode in ILP32. The mechanism looks OK but I think the approach >> that 'add_losym' adopts is neater, which checks on the mode instead of >> TARGET_ILP32 and calls gen_add_losym_di or gen_add_losym_si accordingly. >> Note that the iterator used in add_losym_<mode> is P instead of PTR. > > Yes I agree with this and will fix this. > >> >> Same for tlsie_small above. > > But not with this one, tlsie_small should rather be similar to > ldr_got_small instead. Agree; tlsie_small needs to handle the load of an SImode-sized item from an address having DImode. Yufeng
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index b1b4eef..a3e4532 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -628,22 +628,37 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm, case SYMBOL_SMALL_TLSDESC: { - rtx x0 = gen_rtx_REG (Pmode, R0_REGNUM); + enum machine_mode mode = GET_MODE (dest); + rtx x0 = gen_rtx_REG (mode, R0_REGNUM); rtx tp; + gcc_assert (mode == Pmode || mode == ptr_mode); + emit_insn (gen_tlsdesc_small (imm)); tp = aarch64_load_tp (NULL); - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, x0))); + + if (mode != Pmode) + tp = gen_lowpart (mode, tp); + + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, x0))); set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); return; } case SYMBOL_SMALL_GOTTPREL: { - rtx tmp_reg = gen_reg_rtx (Pmode); + enum machine_mode mode = GET_MODE (dest); + rtx tmp_reg = gen_reg_rtx (mode); rtx tp = aarch64_load_tp (NULL); + + gcc_assert (mode == Pmode || mode == ptr_mode); + emit_insn (gen_tlsie_small (tmp_reg, imm)); - emit_insn (gen_rtx_SET (Pmode, dest, gen_rtx_PLUS (Pmode, tp, tmp_reg))); + + if (mode != Pmode) + tp = gen_lowpart (mode, tp); + + emit_insn (gen_rtx_SET (mode, dest, gen_rtx_PLUS (mode, tp, tmp_reg))); set_unique_reg_note (get_last_insn (), REG_EQUIV, imm); return; } diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 313517f..08fcc94 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -3577,35 +3577,85 @@ [(set_attr "type" "call") (set_attr "length" "16")]) -(define_insn "tlsie_small" - [(set (match_operand:DI 0 "register_operand" "=r") - (unspec:DI [(match_operand:DI 1 "aarch64_tls_ie_symref" "S")] +(define_expand "tlsie_small" + [(set (match_operand 0 "register_operand" "=r") + (unspec [(match_operand 1 "aarch64_tls_ie_symref" "S")] + UNSPEC_GOTSMALLTLS))] + "" +{ + if (TARGET_ILP32) + { + operands[0] = gen_lowpart (ptr_mode, operands[0]); + emit_insn (gen_tlsie_small_si (operands[0], operands[1])); + } + else + emit_insn (gen_tlsie_small_di (operands[0], operands[1])); + DONE; +}) + +(define_insn "tlsie_small_<mode>" + [(set (match_operand:PTR 0 "register_operand" "=r") + (unspec:PTR [(match_operand 1 "aarch64_tls_ie_symref" "S")] UNSPEC_GOTSMALLTLS))] "" - "adrp\\t%0, %A1\;ldr\\t%0, [%0, #%L1]" + "adrp\\t%0, %A1\;ldr\\t%<w>0, [%0, #%L1]" [(set_attr "type" "load1") (set_attr "length" "8")] ) -(define_insn "tlsle_small" - [(set (match_operand:DI 0 "register_operand" "=r") - (unspec:DI [(match_operand:DI 1 "register_operand" "r") - (match_operand:DI 2 "aarch64_tls_le_symref" "S")] + +(define_expand "tlsle_small" + [(set (match_operand 0 "register_operand" "=r") + (unspec [(match_operand 1 "register_operand" "r") + (match_operand 2 "aarch64_tls_le_symref" "S")] + UNSPEC_GOTSMALLTLS))] + "" +{ + if (TARGET_ILP32) + { + rtx temp = gen_reg_rtx (ptr_mode); + operands[1] = gen_lowpart (ptr_mode, operands[1]); + emit_insn (gen_tlsle_small_si (temp, operands[1], operands[2])); + emit_move_insn (operands[0], gen_lowpart (GET_MODE (operands[0]), temp)); + } + else + emit_insn (gen_tlsle_small_di (operands[0], operands[1], operands[2])); + DONE; + +}) + +(define_insn "tlsle_small_<mode>" + [(set (match_operand:PTR 0 "register_operand" "=r") + (unspec:PTR [(match_operand:PTR 1 "register_operand" "r") + (match_operand 2 "aarch64_tls_le_symref" "S")] UNSPEC_GOTSMALLTLS))] "" - "add\\t%0, %1, #%G2\;add\\t%0, %0, #%L2" + "add\\t%<w>0, %<w>1, #%G2\;add\\t%<w>0, %<w>0, #%L2" [(set_attr "type" "alu_reg") (set_attr "length" "8")] ) -(define_insn "tlsdesc_small" - [(set (reg:DI R0_REGNUM) - (unspec:DI [(match_operand:DI 0 "aarch64_valid_symref" "S")] +(define_expand "tlsdesc_small" + [(set (reg R0_REGNUM) + (unspec [(match_operand 0 "aarch64_valid_symref" "S")] + UNSPEC_TLSDESC))] +"TARGET_TLS_DESC" +{ + if (TARGET_ILP32) + emit_insn (gen_tlsdesc_small_si (operands[0])); + else + emit_insn (gen_tlsdesc_small_di (operands[0])); + DONE; +}) + +(define_insn "tlsdesc_small_<mode>" + [(set (reg:PTR R0_REGNUM) + (unspec:PTR [(match_operand 0 "aarch64_valid_symref" "S")] UNSPEC_TLSDESC)) (clobber (reg:DI LR_REGNUM)) (clobber (match_scratch:DI 1 "=r"))] "TARGET_TLS_DESC" - "adrp\\tx0, %A0\;ldr\\t%1, [x0, #%L0]\;add\\tx0, x0, %L0\;.tlsdesccall\\t%0\;blr\\t%1" + "adrp\\tx0, %A0\;ldr\\t%<w>1, [x0, #%L0]\;add\\t<w>0, <w>0, %L0\;.tlsdesccall\\t%0\;blr\\t%1" [(set_attr "type" "call") (set_attr "length" "16")])