[rs6000] use index form addresses more often for ldbrx/stdbrx

At Segher's suggestion, I looked into changing the predicates on bswapdi2_{load,store}
from memory_operand to indexed_or_indirect_operand and putting some code into bswapdi2
to make the address indirect if it wasn't already.

The motivating case for this was the code I was seeing for the gpr expansion of strncmp.
Before I would typically see something like this:

        addi 9,3,8
        ldbrx 10,0,9
        addi 9,4,8
        ldbrx 8,0,9
        subf. 9,8,10
        bne 0,.L13
        cmpb 10,10,9
        cmpdi 0,10,0
        bne 0,.L9
        addi 9,3,16
        ldbrx 10,0,9
        addi 9,4,16
        ldbrx 8,0,9
        subf. 9,8,10
        bne 0,.L13
        cmpb 10,10,9
        cmpdi 0,10,0
        bne 0,.L9

For each comparison block it is doing the add separately and using 0 for one input
of the ldbrx.

After this change, it is more like this:

        ldbrx 8,3,27
        ldbrx 7,4,27
        cmpb 9,8,9
        cmpb 10,8,7
        orc. 9,9,10
        bne 0,.L13
        ldbrx 8,3,24
        ldbrx 7,4,24
        cmpb 10,8,9
        cmpb 9,8,7
        orc. 9,10,9
        bne 0,.L13


Here it has created temps with constants and hoisted them out of a loop, but I have
other cases where it will update them if there is more register pressure. in either
case the code is more compact and makes full use of the indexed addressing of ldbrx.

Bootstrap/regtest passed on ppc64le targeting power7/power8/power9, ok for trunk?

Thanks!
    Aaron

2018-10-27  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.

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.