Message ID | c35d840f-f5be-d015-cf73-8d2e872f9217@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | [rs6000] use index form addresses more often for ldbrx/stdbrx | expand |
Hi Aaron, On Sat, Oct 27, 2018 at 11:20:01AM -0500, Aaron Sawdey wrote: > --- gcc/config/rs6000/rs6000.md (revision 265393) > +++ gcc/config/rs6000/rs6000.md (working copy) > @@ -2512,9 +2512,27 @@ > if (TARGET_POWERPC64 && TARGET_LDBRX) > { > if (MEM_P (src)) > - emit_insn (gen_bswapdi2_load (dest, src)); > + { > + rtx addr = XEXP (src, 0); > + if (!legitimate_indirect_address_p (addr, reload_completed) > + && !legitimate_indexed_address_p (addr, reload_completed)) Should you use indexed_or_indirect operand instead here? > + { > + addr = force_reg (Pmode, addr); > + src = replace_equiv_address_nv (src, addr); > + } > + emit_insn (gen_bswapdi2_load (dest, src)); > + } You could maybe make this a utility routine as well (in rs6000.c)... Something like force_indexed_or_indirect_mem. So this code will be just if (MEM_P (src)) force_indexed_or_indirect_mem (src); then. Could you try those things please? Segher
On 10/27/18 12:52 PM, Segher Boessenkool wrote: > Hi Aaron, > > On Sat, Oct 27, 2018 at 11:20:01AM -0500, Aaron Sawdey wrote: >> --- gcc/config/rs6000/rs6000.md (revision 265393) >> +++ gcc/config/rs6000/rs6000.md (working copy) >> @@ -2512,9 +2512,27 @@ >> if (TARGET_POWERPC64 && TARGET_LDBRX) >> { >> if (MEM_P (src)) >> - emit_insn (gen_bswapdi2_load (dest, src)); >> + { >> + rtx addr = XEXP (src, 0); >> + if (!legitimate_indirect_address_p (addr, reload_completed) >> + && !legitimate_indexed_address_p (addr, reload_completed)) > > Should you use indexed_or_indirect operand instead here? > >> + { >> + addr = force_reg (Pmode, addr); >> + src = replace_equiv_address_nv (src, addr); >> + } >> + emit_insn (gen_bswapdi2_load (dest, src)); >> + } > > You could maybe make this a utility routine as well (in rs6000.c)... > Something like force_indexed_or_indirect_mem. So this code will be just > > if (MEM_P (src)) > force_indexed_or_indirect_mem (src); > > then. > > Could you try those things please? > > > Segher > Segher, Here's a patch restructured in that way. OK for trunk if bootstrap/regtest passes? Thanks! Aaron 2018-10-29 Aaron Sawdey <acsawdey@linux.ibm.com> * config/rs6000/rs6000.md (bswapdi2): Force address into register if not in one already. (bswapdi2_load): Change predicate to indexed_or_indirect_operand. (bswapdi2_store): Ditto. * config/rs6000/rs6000.c (rs6000_force_indexed_or_indirect_mem): New helper function. * config/rs6000/rs6000-protos.h (rs6000_force_indexed_or_indirect_mem): Prototype for helper function. Index: gcc/config/rs6000/rs6000-protos.h =================================================================== --- gcc/config/rs6000/rs6000-protos.h (revision 265588) +++ gcc/config/rs6000/rs6000-protos.h (working copy) @@ -47,6 +47,7 @@ extern bool legitimate_indirect_address_p (rtx, int); extern bool legitimate_indexed_address_p (rtx, int); extern bool avoiding_indexed_address_p (machine_mode); +extern void rs6000_force_indexed_or_indirect_mem (rtx x); extern rtx rs6000_got_register (rtx); extern rtx find_addr_reg (rtx); Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 265588) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -8423,7 +8423,22 @@ return false; } +/* Helper function for making sure we will make full + use of indexed addressing. */ +void +rs6000_force_indexed_or_indirect_mem (rtx x) +{ + rtx addr = XEXP (x, 0); + machine_mode m = GET_MODE (x); + if (!indexed_or_indirect_operand (x, m)) + { + addr = force_reg (Pmode, addr); + x = replace_equiv_address_nv (x, addr); + } +} + + /* Implement the TARGET_LEGITIMATE_COMBINED_INSN hook. */ static bool Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 265588) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -2512,9 +2512,15 @@ if (TARGET_POWERPC64 && TARGET_LDBRX) { if (MEM_P (src)) - emit_insn (gen_bswapdi2_load (dest, src)); + { + rs6000_force_indexed_or_indirect_mem (src); + emit_insn (gen_bswapdi2_load (dest, src)); + } else if (MEM_P (dest)) - emit_insn (gen_bswapdi2_store (dest, src)); + { + rs6000_force_indexed_or_indirect_mem (dest); + emit_insn (gen_bswapdi2_store (dest, src)); + } else if (TARGET_P9_VECTOR) emit_insn (gen_bswapdi2_xxbrd (dest, src)); else @@ -2535,13 +2541,13 @@ ;; Power7/cell has ldbrx/stdbrx, so use it directly (define_insn "bswapdi2_load" [(set (match_operand:DI 0 "gpc_reg_operand" "=r") - (bswap:DI (match_operand:DI 1 "memory_operand" "Z")))] + (bswap:DI (match_operand:DI 1 "indexed_or_indirect_operand" "Z")))] "TARGET_POWERPC64 && TARGET_LDBRX" "ldbrx %0,%y1" [(set_attr "type" "load")]) (define_insn "bswapdi2_store" - [(set (match_operand:DI 0 "memory_operand" "=Z") + [(set (match_operand:DI 0 "indexed_or_indirect_operand" "=Z") (bswap:DI (match_operand:DI 1 "gpc_reg_operand" "r")))] "TARGET_POWERPC64 && TARGET_LDBRX" "stdbrx %1,%y0"
Hi again, On Mon, Oct 29, 2018 at 09:39:59AM -0500, Aaron Sawdey wrote: > * config/rs6000/rs6000.md (bswapdi2): Force address into register > if not in one already. This isn't very correct, could you rephrase? > +void > +rs6000_force_indexed_or_indirect_mem (rtx x) > +{ > + rtx addr = XEXP (x, 0); You can move this line down a block. > + machine_mode m = GET_MODE (x); > + if (!indexed_or_indirect_operand (x, m)) > + { (to here) > + addr = force_reg (Pmode, addr); > + x = replace_equiv_address_nv (x, addr); > + } > +} Okay for trunk (if it works ;-) ) Thanks! Segher
I had to make one more change to make this actually work. In rs6000_force_indexed_or_indirect_mem() it was necessary to return the updated rtx. Bootstrap/regtest passes on ppc64le (power7, power9), ok for trunk? Thanks! Aaron 2018-10-30 Aaron Sawdey <acsawdey@linux.ibm.com> * config/rs6000/rs6000.md (bswapdi2): Force address into register if not in indexed or indirect form. (bswapdi2_load): Change predicate to indexed_or_indirect_operand. (bswapdi2_store): Ditto. * config/rs6000/rs6000.c (rs6000_force_indexed_or_indirect_mem): New helper function. * config/rs6000/rs6000-protos.h (rs6000_force_indexed_or_indirect_mem): Prototype for helper function. Index: gcc/config/rs6000/rs6000-protos.h =================================================================== --- gcc/config/rs6000/rs6000-protos.h (revision 265588) +++ gcc/config/rs6000/rs6000-protos.h (working copy) @@ -47,6 +47,7 @@ extern bool legitimate_indirect_address_p (rtx, int); extern bool legitimate_indexed_address_p (rtx, int); extern bool avoiding_indexed_address_p (machine_mode); +extern rtx rs6000_force_indexed_or_indirect_mem (rtx x); extern rtx rs6000_got_register (rtx); extern rtx find_addr_reg (rtx); Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 265588) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -8423,7 +8423,23 @@ return false; } +/* Helper function for making sure we will make full + use of indexed addressing. */ +rtx +rs6000_force_indexed_or_indirect_mem (rtx x) +{ + machine_mode m = GET_MODE (x); + if (!indexed_or_indirect_operand (x, m)) + { + rtx addr = XEXP (x, 0); + addr = force_reg (Pmode, addr); + x = replace_equiv_address_nv (x, addr); + } + return x; +} + + /* Implement the TARGET_LEGITIMATE_COMBINED_INSN hook. */ static bool Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 265588) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -2512,9 +2512,15 @@ if (TARGET_POWERPC64 && TARGET_LDBRX) { if (MEM_P (src)) - emit_insn (gen_bswapdi2_load (dest, src)); + { + src = rs6000_force_indexed_or_indirect_mem (src); + emit_insn (gen_bswapdi2_load (dest, src)); + } else if (MEM_P (dest)) - emit_insn (gen_bswapdi2_store (dest, src)); + { + dest = rs6000_force_indexed_or_indirect_mem (dest); + emit_insn (gen_bswapdi2_store (dest, src)); + } else if (TARGET_P9_VECTOR) emit_insn (gen_bswapdi2_xxbrd (dest, src)); else @@ -2535,13 +2541,13 @@ ;; Power7/cell has ldbrx/stdbrx, so use it directly (define_insn "bswapdi2_load" [(set (match_operand:DI 0 "gpc_reg_operand" "=r") - (bswap:DI (match_operand:DI 1 "memory_operand" "Z")))] + (bswap:DI (match_operand:DI 1 "indexed_or_indirect_operand" "Z")))] "TARGET_POWERPC64 && TARGET_LDBRX" "ldbrx %0,%y1" [(set_attr "type" "load")]) (define_insn "bswapdi2_store" - [(set (match_operand:DI 0 "memory_operand" "=Z") + [(set (match_operand:DI 0 "indexed_or_indirect_operand" "=Z") (bswap:DI (match_operand:DI 1 "gpc_reg_operand" "r")))] "TARGET_POWERPC64 && TARGET_LDBRX" "stdbrx %1,%y0"
On Tue, Oct 30, 2018 at 10:22:48AM -0500, Aaron Sawdey wrote: > I had to make one more change to make this actually work. In > rs6000_force_indexed_or_indirect_mem() it was necessary to > return the updated rtx. Yes, that probably work better ;-) > Bootstrap/regtest passes on ppc64le (power7, power9), ok for trunk? Okay. Thanks! Segher > 2018-10-30 Aaron Sawdey <acsawdey@linux.ibm.com> > > * config/rs6000/rs6000.md (bswapdi2): Force address into register > if not in indexed or indirect form. > (bswapdi2_load): Change predicate to indexed_or_indirect_operand. > (bswapdi2_store): Ditto. > * config/rs6000/rs6000.c (rs6000_force_indexed_or_indirect_mem): New > helper function. > * config/rs6000/rs6000-protos.h (rs6000_force_indexed_or_indirect_mem): > Prototype for helper function.
Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 265393) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -2512,9 +2512,27 @@ if (TARGET_POWERPC64 && TARGET_LDBRX) { if (MEM_P (src)) - emit_insn (gen_bswapdi2_load (dest, src)); + { + rtx addr = XEXP (src, 0); + if (!legitimate_indirect_address_p (addr, reload_completed) + && !legitimate_indexed_address_p (addr, reload_completed)) + { + addr = force_reg (Pmode, addr); + src = replace_equiv_address_nv (src, addr); + } + emit_insn (gen_bswapdi2_load (dest, src)); + } else if (MEM_P (dest)) - emit_insn (gen_bswapdi2_store (dest, src)); + { + rtx addr = XEXP (dest, 0); + if (!legitimate_indirect_address_p (addr, reload_completed) + && !legitimate_indexed_address_p (addr, reload_completed)) + { + addr = force_reg (Pmode, addr); + dest = replace_equiv_address_nv (dest, addr); + } + emit_insn (gen_bswapdi2_store (dest, src)); + } else if (TARGET_P9_VECTOR) emit_insn (gen_bswapdi2_xxbrd (dest, src)); else @@ -2535,13 +2553,13 @@ ;; Power7/cell has ldbrx/stdbrx, so use it directly (define_insn "bswapdi2_load" [(set (match_operand:DI 0 "gpc_reg_operand" "=r") - (bswap:DI (match_operand:DI 1 "memory_operand" "Z")))] + (bswap:DI (match_operand:DI 1 "indexed_or_indirect_operand" "Z")))] "TARGET_POWERPC64 && TARGET_LDBRX" "ldbrx %0,%y1" [(set_attr "type" "load")]) (define_insn "bswapdi2_store" - [(set (match_operand:DI 0 "memory_operand" "=Z") + [(set (match_operand:DI 0 "indexed_or_indirect_operand" "=Z") (bswap:DI (match_operand:DI 1 "gpc_reg_operand" "r")))] "TARGET_POWERPC64 && TARGET_LDBRX" "stdbrx %1,%y0"