PowerPC V7 future machine patches

PowerPC V7 future machine patches

[Michael Meissner]

This set of patches from V6 got missed, and should go in ASAP.

I am breaking the patches into 3 logical steps:

1) The V7 patches that were part of the V6 patches that have not been
committed.  There are 7 patches in the suite (6 to the compiler, 1 to the
testsuite to enable finer granularity for running the 'future' tests)..

2) The V8 patches will be all of the tests that have been modified.  There are
6 testsuite patches.

3) The V9 patches will be the implementation of PCREL_OPT, which has not been
submitted yet.

I acknowledge that there will be a larger patch after these to clean up the
insn length issues, but none of these patches do anything but add a simple insn
that may or may not be prefixed.  I would prefer to get these out of the way
first.

I have built compilers with these patches, and they have bootstrapped on a
little endian power8 system with no issue.  There have been no regressions in
the test suite.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH] V7, #1 of 7, Use PLI to load up 34-bit DImode constants

[Michael Meissner]

This patch adds an alternative to movdi to allow using PLI (PADDI) to load up
34-bit constants on the 'future' machine.

I have built compilers with this patch, and there were no regressions in the
test suite.  Can I check this into the trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.c (num_insns_constant_gpr): Add support for
	PADDI to load up and/or add 34-bit integer constants.
	(rs6000_rtx_costs): Treat constants loaded up with PADDI with the
	same cost as normal 16-bit constants.
	* config/rs6000/rs6000.md (movdi_internal64): Add support to load
	up 34-bit integer constants with PADDI.
	(movdi integer constant splitter): Add comment about PADDI.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 278173)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -5552,7 +5552,7 @@ static int
 num_insns_constant_gpr (HOST_WIDE_INT value)
 {
   /* signed constant loadable with addi */
-  if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000)
+  if (SIGNED_16BIT_OFFSET_P (value))
     return 1;
 
   /* constant loadable with addis */
@@ -5560,6 +5560,10 @@ num_insns_constant_gpr (HOST_WIDE_INT va
 	   && (value >> 31 == -1 || value >> 31 == 0))
     return 1;
 
+  /* PADDI can support up to 34 bit signed integers.  */
+  else if (TARGET_PREFIXED_ADDR && SIGNED_34BIT_OFFSET_P (value))
+    return 1;
+
   else if (TARGET_POWERPC64)
     {
       HOST_WIDE_INT low  = ((value & 0xffffffff) ^ 0x80000000) - 0x80000000;
@@ -20679,7 +20683,8 @@ rs6000_rtx_costs (rtx x, machine_mode mo
 	    || outer_code == PLUS
 	    || outer_code == MINUS)
 	   && (satisfies_constraint_I (x)
-	       || satisfies_constraint_L (x)))
+	       || satisfies_constraint_L (x)
+	       || satisfies_constraint_eI (x)))
 	  || (outer_code == AND
 	      && (satisfies_constraint_K (x)
 		  || (mode == SImode
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278173)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -8808,24 +8808,24 @@ (define_split
   DONE;
 })
 
-;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR #
-;;              FPR store  FPR load   FPR move   AVX store  AVX store   AVX load
-;;              AVX load   VSX move   P9 0       P9 -1      AVX 0/-1    VSX 0
-;;              VSX -1     P9 const   AVX const  From SPR   To SPR      SPR<->SPR
-;;              VSX->GPR   GPR->VSX
+;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR pli
+;;              GPR #      FPR store  FPR load   FPR move   AVX store   AVX store
+;;              AVX load   AVX load   VSX move   P9 0       P9 -1       AVX 0/-1
+;;              VSX 0      VSX -1     P9 const   AVX const  From SPR    To SPR
+;;              SPR<->SPR  VSX->GPR   GPR->VSX
 (define_insn "*movdi_internal64"
   [(set (match_operand:DI 0 "nonimmediate_operand"
                "=YZ,       r,         r,         r,         r,          r,
-                m,         ^d,        ^d,        wY,        Z,          $v,
-                $v,        ^wa,       wa,        wa,        v,          wa,
-                wa,        v,         v,         r,         *h,         *h,
-                ?r,        ?wa")
+                r,         m,         ^d,        ^d,        wY,         Z,
+                $v,        $v,        ^wa,       wa,        wa,         v,
+                wa,        wa,        v,         v,         r,          *h,
+                *h,        ?r,        ?wa")
 	(match_operand:DI 1 "input_operand"
-               "r,         YZ,        r,         I,         L,          nF,
-                ^d,        m,         ^d,        ^v,        $v,         wY,
-                Z,         ^wa,       Oj,        wM,        OjwM,       Oj,
-                wM,        wS,        wB,        *h,        r,          0,
-                wa,        r"))]
+               "r,         YZ,        r,         I,         L,          eI,
+                nF,        ^d,        m,         ^d,        ^v,         $v,
+                wY,        Z,         ^wa,       Oj,        wM,         OjwM,
+                Oj,        wM,        wS,        wB,        *h,         r,
+                0,         wa,        r"))]
   "TARGET_POWERPC64
    && (gpc_reg_operand (operands[0], DImode)
        || gpc_reg_operand (operands[1], DImode))"
@@ -8835,6 +8835,7 @@ (define_insn "*movdi_internal64"
    mr %0,%1
    li %0,%1
    lis %0,%v1
+   li %0,%1
    #
    stfd%U0%X0 %1,%0
    lfd%U1%X1 %0,%1
@@ -8858,26 +8859,28 @@ (define_insn "*movdi_internal64"
    mtvsrd %x0,%1"
   [(set_attr "type"
                "store,      load,	*,         *,         *,         *,
-                fpstore,    fpload,     fpsimple,  fpstore,   fpstore,   fpload,
-                fpload,     veclogical, vecsimple, vecsimple, vecsimple, veclogical,
-                veclogical, vecsimple,  vecsimple, mfjmpr,    mtjmpr,    *,
-                mftgpr,    mffgpr")
+                *,          fpstore,    fpload,    fpsimple,  fpstore,   fpstore,
+                fpload,     fpload,     veclogical,vecsimple, vecsimple, vecsimple,
+                veclogical, veclogical, vecsimple,  vecsimple, mfjmpr,   mtjmpr,
+                *,          mftgpr,    mffgpr")
    (set_attr "size" "64")
    (set_attr "length"
-               "*,         *,         *,         *,         *,          20,
-                *,         *,         *,         *,         *,          *,
+               "*,         *,         *,         *,         *,          *,
+                20,        *,         *,         *,         *,          *,
                 *,         *,         *,         *,         *,          *,
-                *,         8,         *,         *,         *,          *,
-                *,         *")
+                *,         *,         8,         *,         *,          *,
+                *,         *,         *")
    (set_attr "isa"
-               "*,         *,         *,         *,         *,          *,
-                *,         *,         *,         p9v,       p7v,        p9v,
-                p7v,       *,         p9v,       p9v,       p7v,        *,
-                *,         p7v,       p7v,       *,         *,          *,
-                p8v,       p8v")])
+               "*,         *,         *,         *,         *,          fut,
+                *,         *,         *,         *,         p9v,        p7v,
+                p9v,       p7v,       *,         p9v,       p9v,        p7v,
+                *,         *,         p7v,       p7v,       *,          *,
+                *,         p8v,       p8v")])
 
 ; Some DImode loads are best done as a load of -1 followed by a mask
-; instruction.
+; instruction.  On systems that support the PADDI (PLI) instruction,
+; num_insns_constant returns 1, so these splitter would not be used for things
+; that be loaded with PLI.
 (define_split
   [(set (match_operand:DI 0 "int_reg_operand_not_pseudo")
 	(match_operand:DI 1 "const_int_operand"))]


-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Michael Meissner]

This patch generates the PLI (PADDI) instruction to load up 32-bit SImode
constants on the future system.  It adds an alternative to movsi, and prevents
the movsi load immediate from being split.

I have built compilers with this patch which bootstrapped fine, and there were
no regressions in the test suite.  Can I check this into the trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.md (movsi_internal1): Add support to load
	up 32-bit SImode integer constants with PADDI.
	(movsi integer constant splitter): Do not split constant if PADDI
	can load it up directly.

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278175)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -6891,22 +6891,22 @@ (define_split
 
 ;;		MR           LA           LWZ          LFIWZX       LXSIWZX
 ;;		STW          STFIWX       STXSIWX      LI           LIS
-;;		#            XXLOR        XXSPLTIB 0   XXSPLTIB -1  VSPLTISW
-;;		XXLXOR 0     XXLORC -1    P9 const     MTVSRWZ      MFVSRWZ
-;;		MF%1         MT%0         NOP
+;;		PLI          #            XXLOR        XXSPLTIB 0   XXSPLTIB -1
+;;		VSPLTISW     XXLXOR 0     XXLORC -1    P9 const     MTVSRWZ
+;;		MFVSRWZ      MF%1         MT%0         NOP
 (define_insn "*movsi_internal1"
   [(set (match_operand:SI 0 "nonimmediate_operand"
 		"=r,         r,           r,           d,           v,
 		 m,          Z,           Z,           r,           r,
-		 r,          wa,          wa,          wa,          v,
-		 wa,         v,           v,           wa,          r,
-		 r,          *h,          *h")
+		 r,          r,           wa,          wa,          wa,
+		 v,          wa,          v,           v,           wa,
+		 r,          r,           *h,          *h")
 	(match_operand:SI 1 "input_operand"
 		"r,          U,           m,           Z,           Z,
 		 r,          d,           v,           I,           L,
-		 n,          wa,          O,           wM,          wB,
-		 O,          wM,          wS,          r,           wa,
-		 *h,         r,           0"))]
+		 eI,         n,           wa,          O,           wM,
+		 wB,         O,           wM,          wS,          r,
+		 wa,         *h,          r,           0"))]
   "gpc_reg_operand (operands[0], SImode)
    || gpc_reg_operand (operands[1], SImode)"
   "@
@@ -6920,6 +6920,7 @@ (define_insn "*movsi_internal1"
    stxsiwx %x1,%y0
    li %0,%1
    lis %0,%v1
+   li %0,%1
    #
    xxlor %x0,%x1,%x1
    xxspltib %x0,0
@@ -6936,21 +6937,21 @@ (define_insn "*movsi_internal1"
   [(set_attr "type"
 		"*,          *,           load,        fpload,      fpload,
 		 store,      fpstore,     fpstore,     *,           *,
-		 *,          veclogical,  vecsimple,   vecsimple,   vecsimple,
-		 veclogical, veclogical,  vecsimple,   mffgpr,      mftgpr,
-		 *,          *,           *")
+		 *,          *,           veclogical,  vecsimple,   vecsimple,
+		 vecsimple,  veclogical,  veclogical,  vecsimple,   mffgpr,
+		 mftgpr,     *,           *,           *")
    (set_attr "length"
 		"*,          *,           *,           *,           *,
 		 *,          *,           *,           *,           *,
-		 8,          *,           *,           *,           *,
-		 *,          *,           8,           *,           *,
-		 *,          *,           *")
+		 *,          8,           *,           *,           *,
+		 *,          *,           *,           8,           *,
+		 *,          *,           *,           *")
    (set_attr "isa"
 		"*,          *,           *,           p8v,         p8v,
 		 *,          p8v,         p8v,         *,           *,
-		 *,          p8v,         p9v,         p9v,         p8v,
-		 p9v,        p8v,         p9v,         p8v,         p8v,
-		 *,          *,           *")])
+		 fut,        *,           p8v,         p9v,         p9v,
+		 p8v,        p9v,         p8v,         p9v,         p8v,
+		 p8v,        *,           *,           *")])
 
 ;; Like movsi, but adjust a SF value to be used in a SI context, i.e.
 ;; (set (reg:SI ...) (subreg:SI (reg:SF ...) 0))
@@ -7095,14 +7096,15 @@ (define_insn "*movsi_from_df"
   "xscvdpsp %x0,%x1"
   [(set_attr "type" "fp")])
 
-;; Split a load of a large constant into the appropriate two-insn
-;; sequence.
+;; Split a load of a large constant into the appropriate two-insn sequence.  On
+;; systems that support PADDI (PLI), we can use PLI to load any 32-bit constant
+;; in one instruction.
 
 (define_split
   [(set (match_operand:SI 0 "gpc_reg_operand")
 	(match_operand:SI 1 "const_int_operand"))]
   "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
-   && (INTVAL (operands[1]) & 0xffff) != 0"
+   && (INTVAL (operands[1]) & 0xffff) != 0 && !TARGET_PREFIXED_ADDR"
   [(set (match_dup 0)
 	(match_dup 2))
    (set (match_dup 0)

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH], V7, #3 of 7, Use PADDI for 34-bit immediate adds

[Michael Meissner]

This patch generates PADDI to add 34-bit immediate constants on the 'future'
system, and prevents such adds from being split.

I have built and boostrapped compilers with the patch, and there were no
regressions.  Can I check this into the trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (add_operand): Add support for
	PADDI.
	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.

Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 278173)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -839,7 +839,8 @@ (define_special_predicate "indexed_addre
 (define_predicate "add_operand"
   (if_then_else (match_code "const_int")
     (match_test "satisfies_constraint_I (op)
-		 || satisfies_constraint_L (op)")
+		 || satisfies_constraint_L (op)
+		 || satisfies_constraint_eI (op)")
     (match_operand 0 "gpc_reg_operand")))
 
 ;; Return 1 if the operand is either a non-special register, or 0, or -1.
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278176)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -1761,15 +1761,17 @@ (define_expand "add<mode>3"
 })
 
 (define_insn "*add<mode>3"
-  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r")
-	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b")
-		  (match_operand:GPR 2 "add_operand" "r,I,L")))]
+  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r,r")
+	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b,b")
+		  (match_operand:GPR 2 "add_operand" "r,I,L,eI")))]
   ""
   "@
    add %0,%1,%2
    addi %0,%1,%2
-   addis %0,%1,%v2"
-  [(set_attr "type" "add")])
+   addis %0,%1,%v2
+   addi %0,%1,%2"
+  [(set_attr "type" "add")
+   (set_attr "isa" "*,*,*,fut")])
 
 (define_insn "*addsi3_high"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=b")

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH] V7, #4 of 7, Add explicit (0),1 to @pcrel references

[Michael Meissner]

In some of my previous work, I had make a mistake forgetting that the PADDI
instruction did not allow adding a PC-relative reference to a register (you can
either load up a PC-relative address without adding a register, or you can add
a register to a constant).  The assembler allowed the instruction, but it
didn't do what I expected.

This patch adds an explicit (0),1 to PC-relative references.  This way if you
try to add the PC-relative offset to a register, it will get a syntax error.

I have built compilers with this patch installed on a little endian power8
Linux system, and there were no regressions.  Can I check this into the trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.c (print_operand_address): Add (0),1 to
	@pcrel to catch errant usage.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 278175)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -13241,7 +13241,10 @@ print_operand_address (FILE *file, rtx x
       if (SYMBOL_REF_P (x) && !SYMBOL_REF_LOCAL_P (x))
 	fprintf (file, "@got");
 
-      fprintf (file, "@pcrel");
+      /* Specifically add (0),1 to catch uses where a @pcrel was added to a an
+	 address with a base register, since the hardware does not support
+	 adding a base register to a PC-relative address.  */
+      fprintf (file, "@pcrel(0),1");
     }
   else if (SYMBOL_REF_P (x) || GET_CODE (x) == CONST
 	   || GET_CODE (x) == LABEL_REF)

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH], V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.

[Michael Meissner]

This patch adds more effective targets to the target-supports.exp in the
testsuite.  I tried to break it down to whether prefixed instructions are
allowed, whether the target is generating 64-bit code with prefixed
instructions, and if -mpcrel support is available.  I also enabled 'future'
testing on the actual hardware (or simulator).

The tests in V8 will use some of these capabilities.

I have run the test suite on a little endian power8 system with no degradation.
Can I check this into the FSF trunk?

2019-11-14   Michael Meissner  <meissner@linux.ibm.com>

	* lib/target-supports.exp
	(check_effective_target_powerpc_future_ok): Do not require 64-bit
	or Linux support before doing the test.  Use a 32-bit constant in
	PLI.
	(check_effective_target_powerpc_prefixed_addr_ok): New effective
	target test to see if prefixed memory instructions are supported.
	(check_effective_target_powerpc_pcrel_ok): New effective target
	test to test whether PC-relative addressing is supported.
	(is-effective-target): Add test for the PowerPC 'future' hardware
	support.

Index: gcc/testsuite/lib/target-supports.exp
===================================================================
--- gcc/testsuite/lib/target-supports.exp	(revision 278173)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -5345,16 +5345,14 @@ proc check_effective_target_powerpc_p9mo
     }
 }
 
-# Return 1 if this is a PowerPC target supporting -mfuture.
-# Limit this to 64-bit linux systems for now until other
-# targets support FUTURE.
+# Return 1 if this is a PowerPC target supporting -mcpu=future.
 
 proc check_effective_target_powerpc_future_ok { } {
-    if { ([istarget powerpc64*-*-linux*]) } {
+    if { ([istarget powerpc*-*-*]) } {
 	return [check_no_compiler_messages powerpc_future_ok object {
 	    int main (void) {
 		long e;
-		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
+		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
 		return e;
 	    }
 	} "-mfuture"]
@@ -5363,6 +5361,46 @@ proc check_effective_target_powerpc_futu
     }
 }
 
+# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
+# must support large numeric prefixed addresses by default when -mfuture is
+# used.  We test loading up a large constant to verify that the full 34-bit
+# offset for prefixed instructions is supported and we check for a prefixed
+# load as well.
+
+proc check_effective_target_powerpc_prefixed_addr_ok { } {
+    if { ([istarget powerpc*-*-*]) } {
+	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
+	    int main (void) {
+		extern long l[];
+		long e, e2;
+		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
+		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));
+		return e - e2;
+	    }
+	} "-mfuture"]
+    } else {
+	return 0
+    }
+}
+
+# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must
+# support PC-relative addressing when -mcpu=future is used to pass this test.
+
+proc check_effective_target_powerpc_pcrel_ok { } {
+    if { ([istarget powerpc*-*-*]) } {
+	return [check_no_compiler_messages powerpc_pcrel_ok object {
+	      int main (void) {
+		  static int s __attribute__((__used__));
+		  int e;
+		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
+		  return e;
+	      }
+	  } "-mfuture"]
+      } else {
+	  return 0
+      }
+}
+
 # Return 1 if this is a PowerPC target supporting -mfloat128 via either
 # software emulation on power7/power8 systems or hardware support on power9.
 
@@ -7261,6 +7299,7 @@ proc is-effective-target { arg } {
 	  "named_sections" { set selected [check_named_sections_available] }
 	  "gc_sections"    { set selected [check_gc_sections_available] }
 	  "cxa_atexit"     { set selected [check_cxa_atexit_available] }
+	  "powerpc_future_hw" { set selected [check_powerpc_future_hw_available] }
 	  default          { error "unknown effective target keyword `$arg'" }
 	}
     }

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH], V7, #6 of 7, Fix issues with vector extract and prefixed instructions

[Michael Meissner]

This patch fixes two issues with vector extracts and prefixed instructions.

The first is if you use a vector extract on a vector that is located in memory
and to access the vector, you use a PC-relative address with a veriable index.
I.e.:

	#include <altivec.h>

	static vector int vi;

	int get (int n)
	{
	  return vec_extract (vi, n);
	}

In this case, the current code re-uses the temporary for calculating the offset
of the element to load up the address of the vector, losing the offset.  This
code prevents the combiner from combining loading the vector from memory and
the vector extract if the vector is accessed via a PC-relative address.
Instead, the vector is loaded up into a register, and the variable extract from
a register is done.

I needed to add a new constraint (em) in addition to new predicate functions.
I discovered that with the predicate function alone, the register allocator
would re-create the address.  The constraint prevents this combination.

I also modified the vector extract code to generate a single PC-relative load
if the vector has a PC-relative address and the offset is constant.

I have built a bootstrap compiler with this change, and there were no
regressions in the test suite.  Can I check this into the FSF trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/constraints.md (em constraint): New constraint for
	non-prefixed memory.
	* config/rs6000/predicates.md (non_prefixed_memory): New
	predicate.
	(reg_or_non_prefixed_memory): New predicate.
	* config/rs6000/rs6000.c (rs6000_adjust_vec_address): Add support
	for optimizing extracting a constant vector element from a vector
	that uses a prefixed address.  If the element number is variable
	and the address uses a prefixed address, abort.
	* config/rs6000/vsx.md (vsx_extract_<mode>_var, VSX_D iterator):
	Do not allow combining prefixed memory with a variable vector
	extract.
	(vsx_extract_v4sf_var): Do not allow combining prefixed memory
	with a variable vector extract.
	(vsx_extract_<mode>_var, VSX_EXTRACT_I iterator): Do not allow
	combining prefixed memory with a variable vector extract.
	(vsx_extract_<mode>_<VS_scalar>mode_var): Do not allow combining
	prefixed memory with a variable vector extract.
	* doc/md.texi (PowerPC constraints): Document the em constraint.

Index: gcc/config/rs6000/constraints.md
===================================================================
--- gcc/config/rs6000/constraints.md	(revision 278173)
+++ gcc/config/rs6000/constraints.md	(working copy)
@@ -202,6 +202,11 @@ (define_constraint "H"
 
 ;; Memory constraints
 
+(define_memory_constraint "em"
+  "A memory operand that does not contain a prefixed address."
+  (and (match_code "mem")
+       (match_test "non_prefixed_memory (op, mode)")))
+
 (define_memory_constraint "es"
   "A ``stable'' memory operand; that is, one which does not include any
 automodification of the base register.  Unlike @samp{m}, this constraint
Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 278177)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -1836,3 +1836,24 @@ (define_predicate "prefixed_memory"
 {
   return address_is_prefixed (XEXP (op, 0), mode, NON_PREFIXED_DEFAULT);
 })
+
+;; Return true if the operand is a memory address that does not use a prefixed
+;; address.
+(define_predicate "non_prefixed_memory"
+  (match_code "mem")
+{
+  /* If the operand is not a valid memory operand even if it is not prefixed,
+     do not return true.  */
+  if (!memory_operand (op, mode))
+    return false;
+
+  return !address_is_prefixed (XEXP (op, 0), mode, NON_PREFIXED_DEFAULT);
+})
+
+;; Return true if the operand is either a register or it is a non-prefixed
+;; memory operand.
+(define_predicate "reg_or_non_prefixed_memory"
+  (match_code "reg,subreg,mem")
+{
+  return gpc_reg_operand (op, mode) || non_prefixed_memory (op, mode);
+})
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 278178)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -6734,6 +6734,7 @@ rs6000_adjust_vec_address (rtx scalar_re
   rtx element_offset;
   rtx new_addr;
   bool valid_addr_p;
+  bool pcrel_p = pcrel_local_address (addr, Pmode);
 
   /* Vector addresses should not have PRE_INC, PRE_DEC, or PRE_MODIFY.  */
   gcc_assert (GET_RTX_CLASS (GET_CODE (addr)) != RTX_AUTOINC);
@@ -6771,6 +6772,38 @@ rs6000_adjust_vec_address (rtx scalar_re
   else if (REG_P (addr) || SUBREG_P (addr))
     new_addr = gen_rtx_PLUS (Pmode, addr, element_offset);
 
+  /* Optimize PC-relative addresses with a constant offset.  */
+  else if (pcrel_p && CONST_INT_P (element_offset))
+    {
+      rtx addr2 = addr;
+      HOST_WIDE_INT offset = INTVAL (element_offset);
+
+      if (GET_CODE (addr2) == CONST)
+	addr2 = XEXP (addr2, 0);
+
+      if (GET_CODE (addr2) == PLUS)
+	{
+	  offset += INTVAL (XEXP (addr2, 1));
+	  addr2 = XEXP (addr2, 0);
+	}
+
+      gcc_assert (SIGNED_34BIT_OFFSET_P (offset));
+      if (offset)
+	{
+	  addr2 = gen_rtx_PLUS (Pmode, addr2, GEN_INT (offset));
+	  new_addr = gen_rtx_CONST (Pmode, addr2);
+	}
+      else
+	new_addr = addr2;
+    }
+
+  /* With only one temporary base register, we can't support a PC-relative
+     address added to a variable offset.  This is because the PADDI instruction
+     requires RA to be 0 when doing a PC-relative add (i.e. no register to add
+     to).  */
+  else if (pcrel_p)
+    gcc_unreachable ();
+
   /* Optimize D-FORM addresses with constant offset with a constant element, to
      include the element offset in the address directly.  */
   else if (GET_CODE (addr) == PLUS)
@@ -6785,8 +6818,11 @@ rs6000_adjust_vec_address (rtx scalar_re
 	  HOST_WIDE_INT offset = INTVAL (op1) + INTVAL (element_offset);
 	  rtx offset_rtx = GEN_INT (offset);
 
-	  if (IN_RANGE (offset, -32768, 32767)
-	      && (scalar_size < 8 || (offset & 0x3) == 0))
+	  if (TARGET_PREFIXED_ADDR && SIGNED_34BIT_OFFSET_P (offset))
+	    new_addr = gen_rtx_PLUS (Pmode, op0, offset_rtx);
+
+	  else if (SIGNED_16BIT_OFFSET_P (offset)
+		   && (scalar_size < 8 || (offset & 0x3) == 0))
 	    new_addr = gen_rtx_PLUS (Pmode, op0, offset_rtx);
 	  else
 	    {
@@ -6834,11 +6870,11 @@ rs6000_adjust_vec_address (rtx scalar_re
       new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset);
     }
 
-  /* If we have a PLUS, we need to see whether the particular register class
-     allows for D-FORM or X-FORM addressing.  */
-  if (GET_CODE (new_addr) == PLUS)
+  /* If we have a PLUS or a PC-relative address without the PLUS, we need to
+     see whether the particular register class allows for D-FORM or X-FORM
+     addressing.  */
+  if (GET_CODE (new_addr) == PLUS || pcrel_p)
     {
-      rtx op1 = XEXP (new_addr, 1);
       addr_mask_type addr_mask;
       unsigned int scalar_regno = reg_or_subregno (scalar_reg);
 
@@ -6855,10 +6891,16 @@ rs6000_adjust_vec_address (rtx scalar_re
       else
 	gcc_unreachable ();
 
-      if (REG_P (op1) || SUBREG_P (op1))
-	valid_addr_p = (addr_mask & RELOAD_REG_INDEXED) != 0;
-      else
+      if (pcrel_p)
 	valid_addr_p = (addr_mask & RELOAD_REG_OFFSET) != 0;
+      else
+	{
+	  rtx op1 = XEXP (new_addr, 1);
+	  if (REG_P (op1) || SUBREG_P (op1))
+	    valid_addr_p = (addr_mask & RELOAD_REG_INDEXED) != 0;
+	  else
+	    valid_addr_p = (addr_mask & RELOAD_REG_OFFSET) != 0;
+	}
     }
 
   else if (REG_P (new_addr) || SUBREG_P (new_addr))
Index: gcc/config/rs6000/vsx.md
===================================================================
--- gcc/config/rs6000/vsx.md	(revision 278173)
+++ gcc/config/rs6000/vsx.md	(working copy)
@@ -3248,9 +3248,10 @@ (define_insn "vsx_vslo_<mode>"
 ;; Variable V2DI/V2DF extract
 (define_insn_and_split "vsx_extract_<mode>_var"
   [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=v,wa,r")
-	(unspec:<VS_scalar> [(match_operand:VSX_D 1 "input_operand" "v,m,m")
-			     (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
-			    UNSPEC_VSX_EXTRACT))
+	(unspec:<VS_scalar>
+	 [(match_operand:VSX_D 1 "reg_or_non_prefixed_memory" "v,em,em")
+	  (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
+	 UNSPEC_VSX_EXTRACT))
    (clobber (match_scratch:DI 3 "=r,&b,&b"))
    (clobber (match_scratch:V2DI 4 "=&v,X,X"))]
   "VECTOR_MEM_VSX_P (<MODE>mode) && TARGET_DIRECT_MOVE_64BIT"
@@ -3318,9 +3319,10 @@ (define_insn_and_split "*vsx_extract_v4s
 ;; Variable V4SF extract
 (define_insn_and_split "vsx_extract_v4sf_var"
   [(set (match_operand:SF 0 "gpc_reg_operand" "=wa,wa,?r")
-	(unspec:SF [(match_operand:V4SF 1 "input_operand" "v,m,m")
-		    (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
-		   UNSPEC_VSX_EXTRACT))
+	(unspec:SF
+	 [(match_operand:V4SF 1 "reg_or_non_prefixed_memory" "v,em,em")
+	  (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
+	 UNSPEC_VSX_EXTRACT))
    (clobber (match_scratch:DI 3 "=r,&b,&b"))
    (clobber (match_scratch:V2DI 4 "=&v,X,X"))]
   "VECTOR_MEM_VSX_P (V4SFmode) && TARGET_DIRECT_MOVE_64BIT"
@@ -3681,7 +3683,7 @@ (define_insn_and_split "*vsx_extract_<mo
 (define_insn_and_split "vsx_extract_<mode>_var"
   [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=r,r,r")
 	(unspec:<VS_scalar>
-	 [(match_operand:VSX_EXTRACT_I 1 "input_operand" "v,v,m")
+	 [(match_operand:VSX_EXTRACT_I 1 "reg_or_non_prefixed_memory" "v,v,em")
 	  (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
 	 UNSPEC_VSX_EXTRACT))
    (clobber (match_scratch:DI 3 "=r,r,&b"))
@@ -3701,7 +3703,7 @@ (define_insn_and_split "*vsx_extract_<mo
   [(set (match_operand:<VS_scalar> 0 "gpc_reg_operand" "=r,r,r")
 	(zero_extend:<VS_scalar>
 	 (unspec:<VSX_EXTRACT_I:VS_scalar>
-	  [(match_operand:VSX_EXTRACT_I 1 "input_operand" "v,v,m")
+	  [(match_operand:VSX_EXTRACT_I 1 "reg_or_non_prefixed_memory" "v,v,em")
 	   (match_operand:DI 2 "gpc_reg_operand" "r,r,r")]
 	  UNSPEC_VSX_EXTRACT)))
    (clobber (match_scratch:DI 3 "=r,r,&b"))
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi	(revision 278173)
+++ gcc/doc/md.texi	(working copy)
@@ -3373,6 +3373,9 @@ asm ("st %1,%0" : "=m<>" (mem) : "r" (va
 
 is not.
 
+@item em
+A memory operand that does not contain a prefixed address.
+
 @item es
 A ``stable'' memory operand; that is, one which does not include any
 automodification of the base register.  This used to be useful when

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

[PATCH], V7, #7 of 7, Turn on -mpcrel for Linux 64-bit, but not for other targets

[Michael Meissner]

This patch will enable prefixed addressing and PC-relative addressing if the OS
target support indicates that the OS supports either prefixed addressing and
whether it supports PC-relative addressing when the user uses -mcpu=future.  At
the moment, 64-bit Linux is the only system that enables both prefixed
addressing and PC-relative addressing.

I have built bootstrap compilers with this patch and there were no regressions
in the testsuite.  In addition, during development, I set each of the two
options, and built a copiler with it, and I observed that the expected behavior
for the default of whether prefixed addressing an PC-relative support is
enabled.  Can I check this into the FSF trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/linux64.h (TARGET_PREFIXED_ADDR_DEFAULT): Enable
	prefixed addressing by default.
	(TARGET_PCREL_DEFAULT): Enable pc-relative addressing by default.
	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Only
	enable -mprefixed-addr and -mpcrel if the OS tm.h says to enable
	it.
	(ADDRESSING_FUTURE_MASKS): New mask macro.
	(OTHER_FUTURE_MASKS): Use ADDRESSING_FUTURE_MASKS.
	* config/rs6000/rs6000.c (TARGET_PREFIXED_ADDR_DEFAULT): Do not
	enable -mprefixed-addr unless the OS tm.h says to.
	(TARGET_PCREL_DEFAULT): Do not enable -mpcrel unless the OS tm.h
	says to.
	(rs6000_option_override_internal): Do not enable -mprefixed-addr
	or -mpcrel unless the OS tm.h says to enable it.  Add more checks
	for -mcpu=future.

Index: gcc/config/rs6000/linux64.h
===================================================================
--- gcc/config/rs6000/linux64.h	(revision 278173)
+++ gcc/config/rs6000/linux64.h	(working copy)
@@ -640,3 +640,11 @@ extern int dot_symbols;
    enabling the __float128 keyword.  */
 #undef	TARGET_FLOAT128_ENABLE_TYPE
 #define TARGET_FLOAT128_ENABLE_TYPE 1
+
+/* Enable support for pc-relative and numeric prefixed addressing on the
+   'future' system.  */
+#undef  TARGET_PREFIXED_ADDR_DEFAULT
+#define TARGET_PREFIXED_ADDR_DEFAULT	1
+
+#undef  TARGET_PCREL_DEFAULT
+#define TARGET_PCREL_DEFAULT		1
Index: gcc/config/rs6000/rs6000-cpus.def
===================================================================
--- gcc/config/rs6000/rs6000-cpus.def	(revision 278173)
+++ gcc/config/rs6000/rs6000-cpus.def	(working copy)
@@ -75,15 +75,21 @@
 				 | OPTION_MASK_P8_VECTOR		\
 				 | OPTION_MASK_P9_VECTOR)
 
-/* Support for a future processor's features.  Do not enable -mpcrel until it
-   is fully functional.  */
+/* Support for a future processor's features.  The prefixed and pc-relative
+   addressing bits are not added here.  Instead, rs6000.c adds them if the OS
+   tm.h says that it supports the addressing modes.  */
 #define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\
-				 | OPTION_MASK_FUTURE			\
+				 | OPTION_MASK_FUTURE)
+
+/* Addressing related flags on a future processor.  These flags are broken out
+   because not all targets will support either pc-relative addressing, or even
+   prefixed addressing, and we want to clear all of the addressing bits
+   on targets that cannot support prefixed/pcrel addressing.  */
+#define ADDRESSING_FUTURE_MASKS	(OPTION_MASK_PCREL			\
 				 | OPTION_MASK_PREFIXED_ADDR)
 
 /* Flags that need to be turned off if -mno-future.  */
-#define OTHER_FUTURE_MASKS	(OPTION_MASK_PCREL			\
-				 | OPTION_MASK_PREFIXED_ADDR)
+#define OTHER_FUTURE_MASKS	ADDRESSING_FUTURE_MASKS
 
 /* Flags that need to be turned off if -mno-power9-vector.  */
 #define OTHER_P9_VECTOR_MASKS	(OPTION_MASK_FLOAT128_HW		\
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 278181)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -98,6 +98,16 @@
 #endif
 #endif
 
+/* Set up the defaults for whether prefixed addressing is used, and if it is
+   used, whether we want to turn on pc-relative support by default.  */
+#ifndef TARGET_PREFIXED_ADDR_DEFAULT
+#define TARGET_PREFIXED_ADDR_DEFAULT	0
+#endif
+
+#ifndef TARGET_PCREL_DEFAULT
+#define TARGET_PCREL_DEFAULT		0
+#endif
+
 /* Support targetm.vectorize.builtin_mask_for_load.  */
 GTY(()) tree altivec_builtin_mask_for_load;
 
@@ -2535,6 +2545,14 @@ rs6000_debug_reg_global (void)
   if (TARGET_DIRECT_MOVE_128)
     fprintf (stderr, DEBUG_FMT_D, "VSX easy 64-bit mfvsrld element",
 	     (int)VECTOR_ELEMENT_MFVSRLD_64BIT);
+
+  if (TARGET_FUTURE)
+    {
+      fprintf (stderr, DEBUG_FMT_D, "TARGET_PREFIXED_ADDR_DEFAULT",
+	       TARGET_PREFIXED_ADDR_DEFAULT);
+      fprintf (stderr, DEBUG_FMT_D, "TARGET_PCREL_DEFAULT",
+	       TARGET_PCREL_DEFAULT);
+    }
 }
 
 \f
@@ -4015,26 +4033,6 @@ rs6000_option_override_internal (bool gl
       rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW;
     }
 
-  /* -mprefixed-addr (and hence -mpcrel) requires -mcpu=future.  */
-  if (TARGET_PREFIXED_ADDR && !TARGET_FUTURE)
-    {
-      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
-	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");
-      else if ((rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED_ADDR) != 0)
-	error ("%qs requires %qs", "-mprefixed-addr", "-mcpu=future");
-
-      rs6000_isa_flags &= ~(OPTION_MASK_PCREL | OPTION_MASK_PREFIXED_ADDR);
-    }
-
-  /* -mpcrel requires prefixed load/store addressing.  */
-  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)
-    {
-      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
-	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");
-
-      rs6000_isa_flags &= ~OPTION_MASK_PCREL;
-    }
-
   /* Print the options after updating the defaults.  */
   if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
     rs6000_print_isa_options (stderr, 0, "after defaults", rs6000_isa_flags);
@@ -4166,12 +4164,89 @@ rs6000_option_override_internal (bool gl
   SUB3TARGET_OVERRIDE_OPTIONS;
 #endif
 
-  /* -mpcrel requires -mcmodel=medium, but we can't check TARGET_CMODEL until
-      after the subtarget override options are done.  */
-  if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
+  /* Enable prefixed addressing and pc-relative addressing on 64-bit ELF v2
+     systems if the OS tm.h file says that it is supported and the user did not
+     explicitly use -mprefixed-addr or -mpcrel.  At the present time, only
+     64-bit Linux enables this.
+
+     Pc-relative support also requires the medium code model.
+
+     However, we can't check for ELFv2 or -mcmodel=medium until after the
+     subtarget macros are run.
+
+     If prefixed addressing is disabled by default, and the user does -mpcrel,
+     don't force them to also specify -mprefixed-addr.  */
+  if (TARGET_FUTURE)
+    {
+      bool explicit_prefixed = ((rs6000_isa_flags_explicit
+				 & OPTION_MASK_PREFIXED_ADDR) != 0);
+      bool explicit_pcrel = ((rs6000_isa_flags_explicit
+			      & OPTION_MASK_PCREL) != 0);
+
+      /* Prefixed addressing requires 64-bit registers.  */
+      if (!TARGET_POWERPC64)
+	{
+	  if (TARGET_PCREL && explicit_pcrel)
+	    error ("%qs requires %qs", "-mpcrel", "-m64");
+
+	  else if (TARGET_PREFIXED_ADDR && explicit_prefixed)
+	    error ("%qs requires %qs", "-mprefixed-addr", "-m64");
+
+	  rs6000_isa_flags &= ~ADDRESSING_FUTURE_MASKS;
+	}
+
+      /* Only ELFv2 currently supports prefixed/pcrel addressing.  */
+      else if (rs6000_current_abi != ABI_ELFv2)
+	{
+	  if (TARGET_PCREL && explicit_pcrel)
+	    error ("%qs requires %qs", "-mpcrel", "-mabi=elfv2");
+
+	  else if (TARGET_PREFIXED_ADDR && explicit_prefixed)
+	    error ("%qs requires %qs", "-mprefixed-addr", "-mabi=elfv2");
+
+	  rs6000_isa_flags &= ~ADDRESSING_FUTURE_MASKS;
+	}
+
+      /* Pc-relative requires the medium code model.  */
+      else if (TARGET_PCREL && TARGET_CMODEL != CMODEL_MEDIUM)
+	{
+	  if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
+	    error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
+
+	  rs6000_isa_flags &= ~OPTION_MASK_PCREL;
+	}
+
+      /* Enable defaults if desired.  */
+      else
+	{
+	  if (!explicit_prefixed
+	      && (TARGET_PREFIXED_ADDR_DEFAULT
+		  || TARGET_PCREL
+		  || TARGET_PCREL_DEFAULT))
+	    rs6000_isa_flags |= OPTION_MASK_PREFIXED_ADDR;
+
+	  if (!explicit_pcrel && TARGET_PCREL_DEFAULT
+	      && TARGET_CMODEL == CMODEL_MEDIUM)
+	    rs6000_isa_flags |= OPTION_MASK_PCREL;
+	}
+    }
+
+  /* -mprefixed-addr (and hence -mpcrel) requires -mcpu=future.  */
+  if (TARGET_PREFIXED_ADDR && !TARGET_FUTURE)
     {
       if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
-	error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
+	error ("%qs requires %qs", "-mpcrel", "-mcpu=future");
+      else if ((rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED_ADDR) != 0)
+	error ("%qs requires %qs", "-mprefixed-addr", "-mcpu=future");
+
+      rs6000_isa_flags &= ~(OPTION_MASK_PCREL | OPTION_MASK_PREFIXED_ADDR);
+    }
+
+  /* -mpcrel requires prefixed load/store addressing.  */
+  if (TARGET_PCREL && !TARGET_PREFIXED_ADDR)
+    {
+      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
+	error ("%qs requires %qs", "-mpcrel", "-mprefixed-addr");
 
       rs6000_isa_flags &= ~OPTION_MASK_PCREL;
     }

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] V7, #1 of 7, Use PLI to load up 34-bit DImode constants

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 05:40:10PM -0500, Michael Meissner wrote:
> --- gcc/config/rs6000/rs6000.c	(revision 278173)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -5552,7 +5552,7 @@ static int
>  num_insns_constant_gpr (HOST_WIDE_INT value)
>  {
>    /* signed constant loadable with addi */
> -  if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000)
> +  if (SIGNED_16BIT_OFFSET_P (value))
>      return 1;

Hrm, so the SIGNED_nBIT_OFFSET_P should not be called "offset", if we use
them for other numbers as well.

> -;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR #
> -;;              FPR store  FPR load   FPR move   AVX store  AVX store   AVX load
> -;;              AVX load   VSX move   P9 0       P9 -1      AVX 0/-1    VSX 0
> -;;              VSX -1     P9 const   AVX const  From SPR   To SPR      SPR<->SPR
> -;;              VSX->GPR   GPR->VSX
> +;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR pli
> +;;              GPR #      FPR store  FPR load   FPR move   AVX store   AVX store
> +;;              AVX load   AVX load   VSX move   P9 0       P9 -1       AVX 0/-1
> +;;              VSX 0      VSX -1     P9 const   AVX const  From SPR    To SPR
> +;;              SPR<->SPR  VSX->GPR   GPR->VSX

I cannot make heads or tails of it this way.  Please just add the "pli",
don't rearrange everything else.

There do not have to be exactly six per line.  The only reason to have
some order here is to make it easier to read, not to make it *harder*!

So for this first line let's have three GPR moves, and then have four
load immediates.  Then in the future if we need to edit it again, make
the edited part make some sense, etc.

>  ; Some DImode loads are best done as a load of -1 followed by a mask
> -; instruction.
> +; instruction.  On systems that support the PADDI (PLI) instruction,
> +; num_insns_constant returns 1, so these splitter would not be used for things
> +; that be loaded with PLI.

That comment doesn't add much at all?  This splitter isn't used for
constants we can load in one insn, that's right.  That happily works
just fine if we have prefixed insns as well.


Okay for trunk with those things fixed.  Thanks!


Segher

Re: [PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 05:42:43PM -0500, Michael Meissner wrote:
> -;; Split a load of a large constant into the appropriate two-insn
> -;; sequence.
> +;; Split a load of a large constant into the appropriate two-insn sequence.  On
> +;; systems that support PADDI (PLI), we can use PLI to load any 32-bit constant
> +;; in one instruction.
>  
>  (define_split
>    [(set (match_operand:SI 0 "gpc_reg_operand")
>  	(match_operand:SI 1 "const_int_operand"))]
>    "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
> -   && (INTVAL (operands[1]) & 0xffff) != 0"
> +   && (INTVAL (operands[1]) & 0xffff) != 0 && !TARGET_PREFIXED_ADDR"
>    [(set (match_dup 0)
>  	(match_dup 2))
>     (set (match_dup 0)

Please use num_insns_constant, instead (and fix num_insns_constant_gpr
so it knows about SIGNED_34BIT).


Segher

Re: [PATCH], V7, #3 of 7, Use PADDI for 34-bit immediate adds

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> This patch generates PADDI to add 34-bit immediate constants on the 'future'
> system, and prevents such adds from being split.

I don't see that last part?  Is that a remnant of a previous version of
the patch?

> 	* config/rs6000/predicates.md (add_operand): Add support for
> 	PADDI.

More directly: Allow constants that satisfy eI.

> 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.

(add<mode>3 for GPR): etc.

We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
VEC_F).  Erm.

Wait, this is called *add<mode>3, not add<mode>3.


Okay for trunk with that fixed.  Thanks,


Segher

Re: [PATCH] V7, #4 of 7, Add explicit (0),1 to @pcrel references

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 05:51:14PM -0500, Michael Meissner wrote:
> In some of my previous work, I had make a mistake forgetting that the PADDI
> instruction did not allow adding a PC-relative reference to a register (you can
> either load up a PC-relative address without adding a register, or you can add
> a register to a constant).  The assembler allowed the instruction, but it
> didn't do what I expected.

So, what was the instruction?

> --- gcc/config/rs6000/rs6000.c	(revision 278175)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -13241,7 +13241,10 @@ print_operand_address (FILE *file, rtx x
>        if (SYMBOL_REF_P (x) && !SYMBOL_REF_LOCAL_P (x))
>  	fprintf (file, "@got");
>  
> -      fprintf (file, "@pcrel");
> +      /* Specifically add (0),1 to catch uses where a @pcrel was added to a an
> +	 address with a base register, since the hardware does not support
> +	 adding a base register to a PC-relative address.  */
> +      fprintf (file, "@pcrel(0),1");

But this is print_operand_address, it shouldn't know anything about
specific instructions, it certainly shouldn't print commas and other
fields.

Can you fix this in the caller?


Segher

Re: [PATCH], V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> 	* lib/target-supports.exp
> 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> 	or Linux support before doing the test.  Use a 32-bit constant in
> 	PLI.

You changed from 0x12345 to 0x1234, instead -- why?

> -# Return 1 if this is a PowerPC target supporting -mfuture.
> -# Limit this to 64-bit linux systems for now until other
> -# targets support FUTURE.
> +# Return 1 if this is a PowerPC target supporting -mcpu=future.

"Return 1 if the assembler supports Future instructions."

Please make it explicit that this isn't about the compiler.

>  proc check_effective_target_powerpc_future_ok { } {
> -    if { ([istarget powerpc64*-*-linux*]) } {
> +    if { ([istarget powerpc*-*-*]) } {
>  	return [check_no_compiler_messages powerpc_future_ok object {
>  	    int main (void) {
>  		long e;
> -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
>  		return e;
>  	    }
>  	} "-mfuture"]

You are still passing -mfuture.


> +# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
> +# must support large numeric prefixed addresses by default when -mfuture is
> +# used.  We test loading up a large constant to verify that the full 34-bit
> +# offset for prefixed instructions is supported and we check for a prefixed
> +# load as well.

The comment says one thing.

-mfuture isn't a compiler option...  Well it still is, but that should
be removed.

The actual test uses only 30 bits (and a positive number).  Which is fine,
but the comment is misleading then: the code doesn't test "if the full
34-bit offset is supported".

I don't understand why we test both pli and pld.

> +proc check_effective_target_powerpc_prefixed_addr_ok { } {

The name says another.

> +    if { ([istarget powerpc*-*-*]) } {

This part has no function?  Are there any testcases that test for our
prefixed insns that are compiler for non-powerpc?

If we want this at all, this test shouldn't be nested, just should be
an early-out.

> +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
> +	    int main (void) {
> +		extern long l[];
> +		long e, e2;
> +		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
> +		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));

(should be "b" for the last constraint; and "&l[0]" is usually written
just "l").

> +		return e - e2;
> +	    }
> +	} "-mfuture"]

And the code tests two things.

-mcpu=future, instead?

Does this need to be separate from check_effective_target_powerpc_future_ok
at all?

> +# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must

That is the third selector claiming to test the same thing ("target supports
-mfuture").

> +# support PC-relative addressing when -mcpu=future is used to pass this test.
> +
> +proc check_effective_target_powerpc_pcrel_ok { } {
> +    if { ([istarget powerpc*-*-*]) } {
> +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> +	      int main (void) {
> +		  static int s __attribute__((__used__));
> +		  int e;
> +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> +		  return e;
> +	      }
> +	  } "-mfuture"]
> +      } else {
> +	  return 0
> +      }
> +}

So every assembler will support either all three of these, or none.
Can you simplify this please?


Segher

Re: [PATCH] V7, #1 of 7, Use PLI to load up 34-bit DImode constants

[Michael Meissner]

On Fri, Nov 22, 2019 at 06:06:19PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:40:10PM -0500, Michael Meissner wrote:
> > --- gcc/config/rs6000/rs6000.c	(revision 278173)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -5552,7 +5552,7 @@ static int
> >  num_insns_constant_gpr (HOST_WIDE_INT value)
> >  {
> >    /* signed constant loadable with addi */
> > -  if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000)
> > +  if (SIGNED_16BIT_OFFSET_P (value))
> >      return 1;
> 
> Hrm, so the SIGNED_nBIT_OFFSET_P should not be called "offset", if we use
> them for other numbers as well.
> 
> > -;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR #
> > -;;              FPR store  FPR load   FPR move   AVX store  AVX store   AVX load
> > -;;              AVX load   VSX move   P9 0       P9 -1      AVX 0/-1    VSX 0
> > -;;              VSX -1     P9 const   AVX const  From SPR   To SPR      SPR<->SPR
> > -;;              VSX->GPR   GPR->VSX
> > +;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR pli
> > +;;              GPR #      FPR store  FPR load   FPR move   AVX store   AVX store
> > +;;              AVX load   AVX load   VSX move   P9 0       P9 -1       AVX 0/-1
> > +;;              VSX 0      VSX -1     P9 const   AVX const  From SPR    To SPR
> > +;;              SPR<->SPR  VSX->GPR   GPR->VSX
> 
> I cannot make heads or tails of it this way.  Please just add the "pli",
> don't rearrange everything else.

You have to put PLI after LI and LIS and before the splitter insn.

> There do not have to be exactly six per line.  The only reason to have
> some order here is to make it easier to read, not to make it *harder*!

But in doing the transformation, I will modify all of the constraint and
attribute lines.  Otherwise it makes it impossible to add new things, and it
makes it impossible to find the appropriate insn.

> So for this first line let's have three GPR moves, and then have four
> load immediates.  Then in the future if we need to edit it again, make
> the edited part make some sense, etc.


So right now we have:

;;              GPR store  GPR load   GPR move   GPR li     GPR lis     GPR #
;;              FPR store  FPR load   FPR move   AVX store  AVX store   AVX load
;;              AVX load   VSX move   P9 0       P9 -1      AVX 0/-1    VSX 0
;;              VSX -1     P9 const   AVX const  From SPR   To SPR      SPR<->SPR
;;              VSX->GPR   GPR->VSX
(define_insn "*movdi_internal64"
  [(set (match_operand:DI 0 "nonimmediate_operand"
               "=YZ,       r,         r,         r,         r,          r,
                m,         ^d,        ^d,        wY,        Z,          $v,
                $v,        ^wa,       wa,        wa,        v,          wa,
                wa,        v,         v,         r,         *h,         *h,
                ?r,        ?wa")
	(match_operand:DI 1 "input_operand"
               "r,         YZ,        r,         I,         L,          nF,
                ^d,        m,         ^d,        ^v,        $v,         wY,
                Z,         ^wa,       Oj,        wM,        OjwM,       Oj,
                wM,        wS,        wB,        *h,        r,          0,
                wa,        r"))]
  "TARGET_POWERPC64
   && (gpc_reg_operand (operands[0], DImode)
       || gpc_reg_operand (operands[1], DImode))"
  "@
   std%U0%X0 %1,%0
   ld%U1%X1 %0,%1
   mr %0,%1
   li %0,%1
   lis %0,%v1
   #
   stfd%U0%X0 %1,%0
   lfd%U1%X1 %0,%1
   fmr %0,%1
   stxsd %1,%0
   stxsdx %x1,%y0
   lxsd %0,%1
   lxsdx %x0,%y1
   xxlor %x0,%x1,%x1
   xxspltib %x0,0
   xxspltib %x0,255
   #
   xxlxor %x0,%x0,%x0
   xxlorc %x0,%x0,%x0
   #
   #
   mf%1 %0
   mt%0 %1
   nop
   mfvsrd %0,%x1
   mtvsrd %x0,%1"
  [(set_attr "type"
               "store,      load,	*,         *,         *,         *,
                fpstore,    fpload,     fpsimple,  fpstore,   fpstore,   fpload,
                fpload,     veclogical, vecsimple, vecsimple, vecsimple, veclogical,
                veclogical, vecsimple,  vecsimple, mfjmpr,    mtjmpr,    *,
                mftgpr,    mffgpr")
   (set_attr "size" "64")
   (set_attr "length"
               "*,         *,         *,         *,         *,          20,
                *,         *,         *,         *,         *,          *,
                *,         *,         *,         *,         *,          *,
                *,         8,         *,         *,         *,          *,
                *,         *")
   (set_attr "isa"
               "*,         *,         *,         *,         *,          *,
                *,         *,         *,         p9v,       p7v,        p9v,
                p7v,       *,         p9v,       p9v,       p7v,        *,
                *,         p7v,       p7v,       *,         *,          *,
                p8v,       p8v")])

If I just change the columns, in order to be able to correlate which constraint
goes with which attribute, it would need to be modified to something like:

;;              GPR store  GPR load   GPR move
;;              GPR li     GPR lis    GPR pli    GPR #
;;              FPR store  FPR load   FPR move
;;              AVX store  AVX store  AVX load   AVX load   VSX move
;;              P9 0       P9 -1      AVX 0/-1   VSX 0      VSX -1
;;              P9 const   AVX const
;;              From SPR   To SPR     SPR<->SPR
;;              VSX->GPR   GPR->VSX

(define_insn "*movdi_internal64"
  [(set (match_operand:DI 0 "nonimmediate_operand"
               "=YZ,       r,         r,
                r,         r,         r,         r,
                m,         ^d,        ^d,
                wY,        Z,         $v,        $v,        ^wa,
                wa,        wa,        v,         wa,        wa,
                v,         v,
                r,         *h,        *h,
                ?r,        ?wa")

	(match_operand:DI 1 "input_operand"
               "r,         YZ,        r,
                I,         L,         eI,        nF,
                ^d,        m,         ^d,        ^v,        $v,
                wY,        Z,         ^wa,       Oj,        wM,
                OjwM,      Oj,        wM,        wS,        wB,
                *h,        r,         0,
                wa,        r"))]

  "TARGET_POWERPC64
   && (gpc_reg_operand (operands[0], DImode)
       || gpc_reg_operand (operands[1], DImode))"
  "@
   std%U0%X0 %1,%0
   ld%U1%X1 %0,%1
   mr %0,%1
   li %0,%1
   lis %0,%v1
   li %0,%1
   #
   stfd%U0%X0 %1,%0
   lfd%U1%X1 %0,%1
   fmr %0,%1
   stxsd %1,%0
   stxsdx %x1,%y0
   lxsd %0,%1
   lxsdx %x0,%y1
   xxlor %x0,%x1,%x1
   xxspltib %x0,0
   xxspltib %x0,255
   #
   xxlxor %x0,%x0,%x0
   xxlorc %x0,%x0,%x0
   #
   #
   mf%1 %0
   mt%0 %1
   nop
   mfvsrd %0,%x1
   mtvsrd %x0,%1"
  [(set_attr "type"
               "store,      load,	*,
                *,          *,          *,         *,
                fpstore,    fpload,    fpsimple,
                fpstore,    fpstore,   fpload,     fpload,     veclogical,
                vecsimple,  vecsimple, vecsimple,  veclogical, veclogical,
                vecsimple,  vecsimple,
                mfjmpr,     mtjmpr,    *,
                mftgpr,     mffgpr")
   (set_attr "size" "64")
   (set_attr "num_insns"
               "*,         *,         *,
                *,         *,         *,           5,
                *,         *,         *,
                *,         *,         *,           *,          *,
                *,         *,         *,           *,          *,
                2,         *,
                *,         *,         *,
                *,         *")
   (set_attr "isa"
               "*,         *,         *,
                *,         *,         fut,         *,
                *,         *,         *,
                p9v,       p7v,       p9v,         p7v,        *,
                p9v,       p9v,       p7v,         *,          *,
                p7v,       p7v,
                *,         *,         *,
                p8v,       p8v")])

Is this what you want?

Now, this is without moving any of the alternatives around.  Logically, you
could move gpr/fpr/vsx moves to be together (and maybe the direct moves also).
But I dunno whether this is getting into bike shedding.
 
> >  ; Some DImode loads are best done as a load of -1 followed by a mask
> > -; instruction.
> > +; instruction.  On systems that support the PADDI (PLI) instruction,
> > +; num_insns_constant returns 1, so these splitter would not be used for things
> > +; that be loaded with PLI.
> 
> That comment doesn't add much at all?  This splitter isn't used for
> constants we can load in one insn, that's right.  That happily works
> just fine if we have prefixed insns as well.

Well you had asked for the comment many months ago.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Michael Meissner]

On Fri, Nov 22, 2019 at 06:20:52PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:42:43PM -0500, Michael Meissner wrote:
> > -;; Split a load of a large constant into the appropriate two-insn
> > -;; sequence.
> > +;; Split a load of a large constant into the appropriate two-insn sequence.  On
> > +;; systems that support PADDI (PLI), we can use PLI to load any 32-bit constant
> > +;; in one instruction.
> >  
> >  (define_split
> >    [(set (match_operand:SI 0 "gpc_reg_operand")
> >  	(match_operand:SI 1 "const_int_operand"))]
> >    "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
> > -   && (INTVAL (operands[1]) & 0xffff) != 0"
> > +   && (INTVAL (operands[1]) & 0xffff) != 0 && !TARGET_PREFIXED_ADDR"
> >    [(set (match_dup 0)
> >  	(match_dup 2))
> >     (set (match_dup 0)
> 
> Please use num_insns_constant, instead (and fix num_insns_constant_gpr
> so it knows about SIGNED_34BIT).

The previous patch V6 #1 already had the modification for
num_insns_constant_gpr.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], V7, #3 of 7, Use PADDI for 34-bit immediate adds

[Michael Meissner]

On Fri, Nov 22, 2019 at 06:32:08PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> > This patch generates PADDI to add 34-bit immediate constants on the 'future'
> > system, and prevents such adds from being split.
> 
> I don't see that last part?  Is that a remnant of a previous version of
> the patch?

This falls out of the add_operand change.

The add splitter is:

;; Split an add that we can't do in one insn into two insns, each of which
;; does one 16-bit part.  This is used by combine.  Note that the low-order
;; add should be last in case the result gets used in an address.

(define_split
  [(set (match_operand:GPR 0 "gpc_reg_operand")
	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand")
		  (match_operand:GPR 2 "non_add_cint_operand")))]
  ""
  [(set (match_dup 0) (plus:GPR (match_dup 1) (match_dup 3)))
   (set (match_dup 0) (plus:GPR (match_dup 0) (match_dup 4)))]
{
  HOST_WIDE_INT val = INTVAL (operands[2]);
  HOST_WIDE_INT low = ((val & 0xffff) ^ 0x8000) - 0x8000;
  HOST_WIDE_INT rest = trunc_int_for_mode (val - low, <MODE>mode);

  operands[4] = GEN_INT (low);
  if (<MODE>mode == SImode || satisfies_constraint_L (GEN_INT (rest)))
    operands[3] = GEN_INT (rest);
  else if (can_create_pseudo_p ())
    {
      operands[3] = gen_reg_rtx (DImode);
      emit_move_insn (operands[3], operands[2]);
      emit_insn (gen_adddi3 (operands[0], operands[1], operands[3]));
      DONE;
    }
  else
    FAIL;
})

Since non_add_cint_operand is defined as:

(define_predicate "non_add_cint_operand"
  (and (match_code "const_int")
       (not (match_operand 0 "add_operand"))))

It follows that if add_operand now allows a 34-bit constant when -mcpu=future,
that non_add_cint_operand will return 0, and the add will not be split.

> > 	* config/rs6000/predicates.md (add_operand): Add support for
> > 	PADDI.
> 
> More directly: Allow constants that satisfy eI.

Ok.

> > 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.
> 
> (add<mode>3 for GPR): etc.
> 
> We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
> VEC_F).  Erm.
> 
> Wait, this is called *add<mode>3, not add<mode>3.

Yeah, I usually add the iterator part to identify which one was changed.
> 
> Okay for trunk with that fixed.  Thanks,
> 
> 
> Segher

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] V7, #4 of 7, Add explicit (0),1 to @pcrel references

[Michael Meissner]

On Fri, Nov 22, 2019 at 06:52:42PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:51:14PM -0500, Michael Meissner wrote:
> > In some of my previous work, I had make a mistake forgetting that the PADDI
> > instruction did not allow adding a PC-relative reference to a register (you can
> > either load up a PC-relative address without adding a register, or you can add
> > a register to a constant).  The assembler allowed the instruction, but it
> > didn't do what I expected.
> 
> So, what was the instruction?

If you write:

	paddi 10,9,foo@pcrel

The assembler will interpret this as:

	paddi 10,9,(.-foo),0

I.e. it will add the difference from current instruction to foo and register 2
and place it in register 10.  But it will not add the current location for the
paddi.

Of course it might make sense if the assembler could flag the first line as an
error, and if you really wanted to write that, you should use:

	paddi 10,9,foo@pcrel,0

> > --- gcc/config/rs6000/rs6000.c	(revision 278175)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -13241,7 +13241,10 @@ print_operand_address (FILE *file, rtx x
> >        if (SYMBOL_REF_P (x) && !SYMBOL_REF_LOCAL_P (x))
> >  	fprintf (file, "@got");
> >  
> > -      fprintf (file, "@pcrel");
> > +      /* Specifically add (0),1 to catch uses where a @pcrel was added to a an
> > +	 address with a base register, since the hardware does not support
> > +	 adding a base register to a PC-relative address.  */
> > +      fprintf (file, "@pcrel(0),1");
> 
> But this is print_operand_address, it shouldn't know anything about
> specific instructions, it certainly shouldn't print commas and other
> fields.
> 
> Can you fix this in the caller?

Since the code was invalid, the add insn modification to allow adding an
address has been removed.  It was a suggestion after I made the mistake to make
sure that nobody else would make that mistake.

As I said, this is an optional patch.  The idea was to be helpful to other
people in the future, in that if they try to generate an add between a register
and a PC-relative label, that the assembler would give a warnging (due to (0),1
present).

If you don't think it is important to do that protection, I can leave it out.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH] V7, #1 of 7, Use PLI to load up 34-bit DImode constants

[Segher Boessenkool]

Hi Mike,

On Mon, Nov 25, 2019 at 05:09:10PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 06:06:19PM -0600, Segher Boessenkool wrote:
> > > --- gcc/config/rs6000/rs6000.c	(revision 278173)
> > > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > > @@ -5552,7 +5552,7 @@ static int
> > >  num_insns_constant_gpr (HOST_WIDE_INT value)
> > >  {
> > >    /* signed constant loadable with addi */
> > > -  if (((unsigned HOST_WIDE_INT) value + 0x8000) < 0x10000)
> > > +  if (SIGNED_16BIT_OFFSET_P (value))
> > >      return 1;
> > 
> > Hrm, so the SIGNED_nBIT_OFFSET_P should not be called "offset", if we use
> > them for other numbers as well.

Please do something about this?  Not in this patch, of course.

> Is this what you want?

Yeah, like that.

Please send a patch doing *only* this typographical change -- no code
changes at all -- before patches that change the code.  Or, alternatively,
just split up lines that you are touching anyway, and eventually we will
get something nice.

Either option is much easier to review than patches moving 60 constraints
around :-)

This is general advice: if you want to rearrange some code, or fix (some
non-trivial) white space, etc., do that in a separate patch *before* the
rest.  Same as with bugfixes.

That way, it can be immediately applied, whether the rest of the patches
needs further revisions or not.  And it makes reviewing the rest a lot
easier too.

> Now, this is without moving any of the alternatives around.  Logically, you
> could move gpr/fpr/vsx moves to be together (and maybe the direct moves also).
> But I dunno whether this is getting into bike shedding.

Yeah you cannot always move things the way you like them, the ordering
also matters for RA itself (sometimes).  It's all not a big deal except
we have so very very many alternatives in the mov patterns.


Segher

Re: [PATCH], V7, #3 of 7, Use PADDI for 34-bit immediate adds

[Segher Boessenkool]

On Mon, Nov 25, 2019 at 05:40:02PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 06:32:08PM -0600, Segher Boessenkool wrote:
> > On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> > > This patch generates PADDI to add 34-bit immediate constants on the 'future'
> > > system, and prevents such adds from being split.
> > 
> > I don't see that last part?  Is that a remnant of a previous version of
> > the patch?
> 
> This falls out of the add_operand change.

<snip>

> Since non_add_cint_operand is defined as:
> 
> (define_predicate "non_add_cint_operand"
>   (and (match_code "const_int")
>        (not (match_operand 0 "add_operand"))))
> 
> It follows that if add_operand now allows a 34-bit constant when -mcpu=future,
> that non_add_cint_operand will return 0, and the add will not be split.

Right, so write that please ("changes add_operand"), not some random
consequence of that change.

> > > 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.
> > 
> > (add<mode>3 for GPR): etc.
> > 
> > We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
> > VEC_F).  Erm.
> > 
> > Wait, this is called *add<mode>3, not add<mode>3.
> 
> Yeah, I usually add the iterator part to identify which one was changed.

No, the asterisk is part of the name, that's my point.  And yes please
say what the iterator is.


Segher

Re: [PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Segher Boessenkool]

On Mon, Nov 25, 2019 at 05:17:08PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 06:20:52PM -0600, Segher Boessenkool wrote:
> > >  (define_split
> > >    [(set (match_operand:SI 0 "gpc_reg_operand")
> > >  	(match_operand:SI 1 "const_int_operand"))]
> > >    "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
> > > -   && (INTVAL (operands[1]) & 0xffff) != 0"
> > > +   && (INTVAL (operands[1]) & 0xffff) != 0 && !TARGET_PREFIXED_ADDR"
> > >    [(set (match_dup 0)
> > >  	(match_dup 2))
> > >     (set (match_dup 0)
> > 
> > Please use num_insns_constant, instead (and fix num_insns_constant_gpr
> > so it knows about SIGNED_34BIT).
> 
> The previous patch V6 #1 already had the modification for
> num_insns_constant_gpr.

It's not in trunk yet?


Segher

Re: [PATCH] V7, #4 of 7, Add explicit (0),1 to @pcrel references

[Segher Boessenkool]

On Mon, Nov 25, 2019 at 05:52:29PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 06:52:42PM -0600, Segher Boessenkool wrote:
> > On Thu, Nov 14, 2019 at 05:51:14PM -0500, Michael Meissner wrote:
> > > In some of my previous work, I had make a mistake forgetting that the PADDI
> > > instruction did not allow adding a PC-relative reference to a register (you can
> > > either load up a PC-relative address without adding a register, or you can add
> > > a register to a constant).  The assembler allowed the instruction, but it
> > > didn't do what I expected.
> > 
> > So, what was the instruction?
> 
> If you write:
> 
> 	paddi 10,9,foo@pcrel
> 
> The assembler will interpret this as:
> 
> 	paddi 10,9,(.-foo),0
> 
> I.e. it will add the difference from current instruction to foo and register 2
> and place it in register 10.  But it will not add the current location for the
> paddi.
> 
> Of course it might make sense if the assembler could flag the first line as an
> error, and if you really wanted to write that, you should use:
> 
> 	paddi 10,9,foo@pcrel,0

Does it ever make sense to allow @pcrel on anything that is not prefixed
and R=1?  (Cc: Alan).

> > Can you fix this in the caller?
> 
> Since the code was invalid, the add insn modification to allow adding an
> address has been removed.  It was a suggestion after I made the mistake to make
> sure that nobody else would make that mistake.
> 
> As I said, this is an optional patch.  The idea was to be helpful to other
> people in the future, in that if they try to generate an add between a register
> and a PC-relative label, that the assembler would give a warnging (due to (0),1
> present).
> 
> If you don't think it is important to do that protection, I can leave it out.

That isn't what I said.  I'm not happy with how you implemented this,
that's all.


Segher

Re: [PATCH] V7, #4 of 7, Add explicit (0),1 to @pcrel references

[Alan Modra]

On Mon, Nov 25, 2019 at 07:05:02PM -0600, Segher Boessenkool wrote:
> On Mon, Nov 25, 2019 at 05:52:29PM -0500, Michael Meissner wrote:
> > If you write:
> > 
> > 	paddi 10,9,foo@pcrel
> > 
> > The assembler will interpret this as:
> > 
> > 	paddi 10,9,(.-foo),0

foo-. actually.

> > I.e. it will add the difference from current instruction to foo and register 2
> > and place it in register 10.  But it will not add the current location for the
> > paddi.
> > 
> > Of course it might make sense if the assembler could flag the first line as an
> > error, and if you really wanted to write that, you should use:
> > 
> > 	paddi 10,9,foo@pcrel,0
> 
> Does it ever make sense to allow @pcrel on anything that is not prefixed
> and R=1?  (Cc: Alan).

Probably not, but we might want something similar to generate the high
part of a 64-bit pcrel offset.

   pli 11,(foo-1f)@highera34
   sldi 11,11,34
1: pla 12,foo@pcrel
   add 12,11,12

We don't have the necessary relocs at the moment, and in any case we
might be better off with other code sequences.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH], V7, #6 of 7, Fix issues with vector extract and prefixed instructions

[Segher Boessenkool]

Hi!

On Thu, Nov 14, 2019 at 06:09:09PM -0500, Michael Meissner wrote:
> In this case, the current code re-uses the temporary for calculating the offset
> of the element to load up the address of the vector, losing the offset.

Reusing the same pseudo is *always* a bad idea.  You get better
optimisation if most code is "SSA-like": write to every pseudo only
once.  Of course you need to violate this where you woule have PHIs in
actual SSA.

> I needed to add a new constraint (em) in addition to new predicate functions.
> I discovered that with the predicate function alone, the register allocator
> would re-create the address.  The constraint prevents this combination.

It's a reasonable thing to have as a contraint, too.

Once again, how should things work in inline assembler?  Can we allow
prefixed addressing there, or should "m" in inline asm mean "em"?

> I also modified the vector extract code to generate a single PC-relative load
> if the vector has a PC-relative address and the offset is constant.

That should be handled by the RTL optimisers anyway, no?  Or is this
for post-reload splitters (a bad idea always).

> 	* config/rs6000/rs6000.c (rs6000_adjust_vec_address): Add support
> 	for optimizing extracting a constant vector element from a vector
> 	that uses a prefixed address.  If the element number is variable
> 	and the address uses a prefixed address, abort.

It doesn't abort.  Erm, wait, it *does* ICE.  Please make that more
prominent (in the code).  It's not clear why you mention it in the
changelog while allt he other details are just "add support"?

I find the control flow very hard to read here.

> +  /* Optimize PC-relative addresses with a constant offset.  */
> +  else if (pcrel_p && CONST_INT_P (element_offset))
> +    {
> +      rtx addr2 = addr;

addr isn't used after this anyway, so you can clobber it just fine.

> +  /* With only one temporary base register, we can't support a PC-relative
> +     address added to a variable offset.  This is because the PADDI instruction
> +     requires RA to be 0 when doing a PC-relative add (i.e. no register to add
> +     to).  */
> +  else if (pcrel_p)
> +    gcc_unreachable ();

That comment suggests we just ICE when we get unwanted input.  Such a
comment belongs where we prevent such code from being formed in the
first place (or nowhere, if it is obvious).


Segher

Re: [PATCH], V7, #7 of 7, Turn on -mpcrel for Linux 64-bit, but not for other targets

[Segher Boessenkool]

On Thu, Nov 14, 2019 at 06:14:47PM -0500, Michael Meissner wrote:
> 	* config/rs6000/linux64.h (TARGET_PREFIXED_ADDR_DEFAULT): Enable
> 	prefixed addressing by default.
> 	(TARGET_PCREL_DEFAULT): Enable pc-relative addressing by default.
> 	* config/rs6000/rs6000-cpus.def (ISA_FUTURE_MASKS_SERVER): Only
> 	enable -mprefixed-addr and -mpcrel if the OS tm.h says to enable
> 	it.
> 	(ADDRESSING_FUTURE_MASKS): New mask macro.
> 	(OTHER_FUTURE_MASKS): Use ADDRESSING_FUTURE_MASKS.
> 	* config/rs6000/rs6000.c (TARGET_PREFIXED_ADDR_DEFAULT): Do not
> 	enable -mprefixed-addr unless the OS tm.h says to.
> 	(TARGET_PCREL_DEFAULT): Do not enable -mpcrel unless the OS tm.h
> 	says to.
> 	(rs6000_option_override_internal): Do not enable -mprefixed-addr
> 	or -mpcrel unless the OS tm.h says to enable it.  Add more checks
> 	for -mcpu=future.

> +/* Support for a future processor's features.  The prefixed and pc-relative
> +   addressing bits are not added here.  Instead, rs6000.c adds them if the OS
> +   tm.h says that it supports the addressing modes.  */

This comment could be a lot clearer.  It should just say it is about
ADRESSING_FUTURE_MASKS, and not mention rs6000.c at all.

>  #define ISA_FUTURE_MASKS_SERVER	(ISA_3_0_MASKS_SERVER			\
> -				 | OPTION_MASK_FUTURE			\
> +				 | OPTION_MASK_FUTURE)
> +
> +/* Addressing related flags on a future processor.  These flags are broken out
> +   because not all targets will support either pc-relative addressing, or even
> +   prefixed addressing, and we want to clear all of the addressing bits
> +   on targets that cannot support prefixed/pcrel addressing.  */
> +#define ADDRESSING_FUTURE_MASKS	(OPTION_MASK_PCREL			\
>  				 | OPTION_MASK_PREFIXED_ADDR)

> --- gcc/config/rs6000/rs6000.c	(revision 278181)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -98,6 +98,16 @@
>  #endif
>  #endif
>  
> +/* Set up the defaults for whether prefixed addressing is used, and if it is
> +   used, whether we want to turn on pc-relative support by default.  */
> +#ifndef TARGET_PREFIXED_ADDR_DEFAULT
> +#define TARGET_PREFIXED_ADDR_DEFAULT	0
> +#endif
> +
> +#ifndef TARGET_PCREL_DEFAULT
> +#define TARGET_PCREL_DEFAULT		0
> +#endif

"DEFAULT" is a bad name for this?  "SUPPORTED"?

We do *not* turn on prefixed or pcrel by default on most linux64 targets
-- most of those are p9 or p8 or p7 or older!  So "SUPPORTED" isn't good
either.  SUPPORTED_BY_OS?

A name like "XXX_DEFAULT" inevitably means we will later get an
"XXX_REALLY_DEFAULT" macro as well.  Not good.

> +  /* Enable prefixed addressing and pc-relative addressing on 64-bit ELF v2
> +     systems if the OS tm.h file says that it is supported and the user did not
> +     explicitly use -mprefixed-addr or -mpcrel.  At the present time, only
> +     64-bit Linux enables this.


> +      /* Enable defaults if desired.  */

This comment does not say anything.  Please say something like "Enable
PCREL and PREFIXED if [...]"

> +      else
> +	{
> +	  if (!explicit_prefixed
> +	      && (TARGET_PREFIXED_ADDR_DEFAULT
> +		  || TARGET_PCREL
> +		  || TARGET_PCREL_DEFAULT))
> +	    rs6000_isa_flags |= OPTION_MASK_PREFIXED_ADDR;
> +
> +	  if (!explicit_pcrel && TARGET_PCREL_DEFAULT
> +	      && TARGET_CMODEL == CMODEL_MEDIUM)
> +	    rs6000_isa_flags |= OPTION_MASK_PCREL;
> +	}
> +    }

... and I don't understand this code.  If you use -mpcrel but you do not
have the medium model, you _do_ get prefixed but you do _not_ get pcrel?
And this all quietly?


Segher

Re: [PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Michael Meissner]

On Mon, Nov 25, 2019 at 06:49:49PM -0600, Segher Boessenkool wrote:
> On Mon, Nov 25, 2019 at 05:17:08PM -0500, Michael Meissner wrote:
> > On Fri, Nov 22, 2019 at 06:20:52PM -0600, Segher Boessenkool wrote:
> > > >  (define_split
> > > >    [(set (match_operand:SI 0 "gpc_reg_operand")
> > > >  	(match_operand:SI 1 "const_int_operand"))]
> > > >    "(unsigned HOST_WIDE_INT) (INTVAL (operands[1]) + 0x8000) >= 0x10000
> > > > -   && (INTVAL (operands[1]) & 0xffff) != 0"
> > > > +   && (INTVAL (operands[1]) & 0xffff) != 0 && !TARGET_PREFIXED_ADDR"
> > > >    [(set (match_dup 0)
> > > >  	(match_dup 2))
> > > >     (set (match_dup 0)
> > > 
> > > Please use num_insns_constant, instead (and fix num_insns_constant_gpr
> > > so it knows about SIGNED_34BIT).
> > 
> > The previous patch V6 #1 already had the modification for
> > num_insns_constant_gpr.
> 
> It's not in trunk yet?

Sorry, the USA Thanksgiving holiday got in the way.

No, the change for num_insns_constant_gpr could not go in until the support for
PLI went in (patch V6 #1).

So for patches V6 #1-3, I believe you approved #1 (movdi) and #3 (addi3/addsi3)
after the changes but not #2 (movsi).  I haven't yet applied the specific bits
of #1 or #3 after doing the reformating patch.  Are the bits for #2 ok with the
above change to use num_insns_constant_gpr.  Or did you want to see the 3
patches once again after the reformating.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.

[Michael Meissner]

On Fri, Nov 22, 2019 at 08:11:16PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> > 	* lib/target-supports.exp
> > 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> > 	or Linux support before doing the test.  Use a 32-bit constant in
> > 	PLI.
> 
> You changed from 0x12345 to 0x1234, instead -- why?

The last time I did the patch there was discussion about 32-bit support.  I
just rewrote the patch to accomidate possibly using -mcpu=future on a 32-bit
platform.  In particular, AIX.  So I rewrote the basic test so that in theory
it would run on a 32-bit platform.

> > -# Return 1 if this is a PowerPC target supporting -mfuture.
> > -# Limit this to 64-bit linux systems for now until other
> > -# targets support FUTURE.
> > +# Return 1 if this is a PowerPC target supporting -mcpu=future.
> 
> "Return 1 if the assembler supports Future instructions."
> 
> Please make it explicit that this isn't about the compiler.

Ok.

> >  proc check_effective_target_powerpc_future_ok { } {
> > -    if { ([istarget powerpc64*-*-linux*]) } {
> > +    if { ([istarget powerpc*-*-*]) } {
> >  	return [check_no_compiler_messages powerpc_future_ok object {
> >  	    int main (void) {
> >  		long e;
> > -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
> >  		return e;
> >  	    }
> >  	} "-mfuture"]
> 
> You are still passing -mfuture.

All of the tests use the -m<flag> test and not -mcpu=<cpu> test
(i.e. -mpower9-vector instead of -mcpu=power9 or -mdejagnu-cpu=power9), so I
was being consistant with those tests.

> > +# Return 1 if this is a PowerPC target supporting -mcpu=future.  The compiler
> > +# must support large numeric prefixed addresses by default when -mfuture is
> > +# used.  We test loading up a large constant to verify that the full 34-bit
> > +# offset for prefixed instructions is supported and we check for a prefixed
> > +# load as well.
> 
> The comment says one thing.
> 
> -mfuture isn't a compiler option...  Well it still is, but that should
> be removed.

Again, I was just being consistant with the other tests.

> The actual test uses only 30 bits (and a positive number).  Which is fine,
> but the comment is misleading then: the code doesn't test "if the full
> 34-bit offset is supported".

I can fix the comment.

> I don't understand why we test both pli and pld.

Just being cautious.

> > +proc check_effective_target_powerpc_prefixed_addr_ok { } {
> 
> The name says another.
> 
> > +    if { ([istarget powerpc*-*-*]) } {
> 
> This part has no function?  Are there any testcases that test for our
> prefixed insns that are compiler for non-powerpc?

Probably not, but all of the other tests have the same prefix.

> If we want this at all, this test shouldn't be nested, just should be
> an early-out.
> 
> > +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {
> > +	    int main (void) {
> > +		extern long l[];
> > +		long e, e2;
> > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345678));
> > +		asm ("pld %0,0x12345678(%1)" : "=r" (e2) : "r" (& l[0]));
> 
> (should be "b" for the last constraint; and "&l[0]" is usually written
> just "l").
> 
> > +		return e - e2;
> > +	    }
> > +	} "-mfuture"]
> 
> And the code tests two things.
> 
> -mcpu=future, instead?
>
> Does this need to be separate from check_effective_target_powerpc_future_ok
> at all?

This gets back to whether some port will eventually want to use FUTURE
instructions in a 32-bit context.  Linux will always be 64-bit little endian,
but there are other platforms out there that may want to generate FUTURE code.

> > +# Return 1 if this is a PowerPC target supporting -mfuture.  The compiler must
> 
> That is the third selector claiming to test the same thing ("target supports
> -mfuture").
> 
> > +# support PC-relative addressing when -mcpu=future is used to pass this test.
> > +
> > +proc check_effective_target_powerpc_pcrel_ok { } {
> > +    if { ([istarget powerpc*-*-*]) } {
> > +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> > +	      int main (void) {
> > +		  static int s __attribute__((__used__));
> > +		  int e;
> > +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> > +		  return e;
> > +	      }
> > +	  } "-mfuture"]
> > +      } else {
> > +	  return 0
> > +      }
> > +}
> 
> So every assembler will support either all three of these, or none.
> Can you simplify this please?

I can imagine for example AIX might support 64-bit 'future' but not support
PC-relative.  I believe you (or David) asked to split up the prefixed
addressing with numeric offets and PC-relative addressing, because ports might
not be able to support PC-relative addressing.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], V7, #6 of 7, Fix issues with vector extract and prefixed instructions

[Michael Meissner]

On Tue, Nov 26, 2019 at 01:20:20PM -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Nov 14, 2019 at 06:09:09PM -0500, Michael Meissner wrote:
> > In this case, the current code re-uses the temporary for calculating the offset
> > of the element to load up the address of the vector, losing the offset.
> 
> Reusing the same pseudo is *always* a bad idea.  You get better
> optimisation if most code is "SSA-like": write to every pseudo only
> once.  Of course you need to violate this where you woule have PHIs in
> actual SSA.

Yes.  I was describing what the current code does (and why I'm fixing it).  It
is a bug.

> > I needed to add a new constraint (em) in addition to new predicate functions.
> > I discovered that with the predicate function alone, the register allocator
> > would re-create the address.  The constraint prevents this combination.
> 
> It's a reasonable thing to have as a contraint, too.
> 
> Once again, how should things work in inline assembler?  Can we allow
> prefixed addressing there, or should "m" in inline asm mean "em"?

At the moment, I think we should just not allow prefixed addresses in asm
constructs at all.

> 
> > I also modified the vector extract code to generate a single PC-relative load
> > if the vector has a PC-relative address and the offset is constant.
> 
> That should be handled by the RTL optimisers anyway, no?  Or is this
> for post-reload splitters (a bad idea always).

You have it backwards.  It is the RTL optimizers that combines the vector
extract with the load in the first place.  My code allows the combination and
generates a single instruction.  Otherwise, it would do a PLA and then do the
vector extract with the address loaded.

> > 	* config/rs6000/rs6000.c (rs6000_adjust_vec_address): Add support
> > 	for optimizing extracting a constant vector element from a vector
> > 	that uses a prefixed address.  If the element number is variable
> > 	and the address uses a prefixed address, abort.
> 
> It doesn't abort.  Erm, wait, it *does* ICE.  Please make that more
> prominent (in the code).  It's not clear why you mention it in the
> changelog while allt he other details are just "add support"?
> 
> I find the control flow very hard to read here.
> 
> > +  /* Optimize PC-relative addresses with a constant offset.  */
> > +  else if (pcrel_p && CONST_INT_P (element_offset))
> > +    {
> > +      rtx addr2 = addr;
> 
> addr isn't used after this anyway, so you can clobber it just fine.

Generally I prefer not to reuse variables.

> > +  /* With only one temporary base register, we can't support a PC-relative
> > +     address added to a variable offset.  This is because the PADDI instruction
> > +     requires RA to be 0 when doing a PC-relative add (i.e. no register to add
> > +     to).  */
> > +  else if (pcrel_p)
> > +    gcc_unreachable ();
> 
> That comment suggests we just ICE when we get unwanted input.  Such a
> comment belongs where we prevent such code from being formed in the
> first place (or nowhere, if it is obvious).

The constraint and predicate is where we prevent it from occuring.  The
gcc_unreachable is just there as insurance that it didn't get recreated later.
During testing before I added the constraint, I found the register allocator
combining the two, even though the predicate didn't allow the combination.  So
the test is just to ICE out if the combination took place.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

Re: [PATCH], V7, #2 of 7, Use PLI to load up 32-bit SImode constants

[Segher Boessenkool]

On Tue, Dec 03, 2019 at 12:57:24PM -0500, Michael Meissner wrote:
> No, the change for num_insns_constant_gpr could not go in until the support for
> PLI went in (patch V6 #1).

Well, I lost track.  So your version 7 to 9 patches do *not* replace the
v6 patches?  Or does "V" mean something else?

Please post patches you propose currently (after addressing comments,
saying what is changed wrt the previous submission, etc.)

And if you restructure the patch sets, e.g. add new things to patches,
I will have to start reviewing from scratch.  While if you just do some
modifications (and you say what you modified, and that matches reality),
and those modifications are in line with previous reviews, then reviewing
makes easy progress.

It also normally would not take a single day even to make such changes.

> So for patches V6 #1-3, I believe you approved #1 (movdi) and #3 (addi3/addsi3)
> after the changes but not #2 (movsi).  I haven't yet applied the specific bits
> of #1 or #3 after doing the reformating patch.  Are the bits for #2 ok with the
> above change to use num_insns_constant_gpr.  Or did you want to see the 3
> patches once again after the reformating.

https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01605.html

This is six weeks ago.  And I do not know if what you call "v6" is this.


Segher

Re: [PATCH], V7, #5 of 7, Add more effective targets for the 'future' system to target-supports.

[Segher Boessenkool]

On Tue, Dec 03, 2019 at 01:11:55PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 08:11:16PM -0600, Segher Boessenkool wrote:
> > On Thu, Nov 14, 2019 at 05:56:50PM -0500, Michael Meissner wrote:
> > > 	* lib/target-supports.exp
> > > 	(check_effective_target_powerpc_future_ok): Do not require 64-bit
> > > 	or Linux support before doing the test.  Use a 32-bit constant in
> > > 	PLI.
> > 
> > You changed from 0x12345 to 0x1234, instead -- why?
> 
> The last time I did the patch there was discussion about 32-bit support.  I
> just rewrote the patch to accomidate possibly using -mcpu=future on a 32-bit
> platform.  In particular, AIX.  So I rewrote the basic test so that in theory
> it would run on a 32-bit platform.

I don't see how this has anything to with it?  The insns was just fine
for 32-bit already?

> > > +# Return 1 if this is a PowerPC target supporting -mcpu=future.

> > >  proc check_effective_target_powerpc_future_ok { } {
> > > -    if { ([istarget powerpc64*-*-linux*]) } {
> > > +    if { ([istarget powerpc*-*-*]) } {
> > >  	return [check_no_compiler_messages powerpc_future_ok object {
> > >  	    int main (void) {
> > >  		long e;
> > > -		asm ("pli %0,%1" : "=r" (e) : "n" (0x12345));
> > > +		asm ("pli %0,%1" : "=r" (e) : "n" (0x1234));
> > >  		return e;
> > >  	    }
> > >  	} "-mfuture"]
> > 
> > You are still passing -mfuture.
> 
> All of the tests use the -m<flag> test and not -mcpu=<cpu> test
> (i.e. -mpower9-vector instead of -mcpu=power9 or -mdejagnu-cpu=power9), so I
> was being consistant with those tests.

Yes, that is a nasty problem, so let's not promulgate that.  Anyway, it
also contradicts the comment on this function.

> > I don't understand why we test both pli and pld.
> 
> Just being cautious.

Hrm, the check_effective_target_powerpc_future_ok test should probably
use a non-prefixed instruction?  If we cannot do that right now, add a
comment?

> > > +proc check_effective_target_powerpc_prefixed_addr_ok { } {
> > 
> > The name says another.
> > 
> > > +    if { ([istarget powerpc*-*-*]) } {
> > 
> > This part has no function?  Are there any testcases that test for our
> > prefixed insns that are compiler for non-powerpc?
> 
> Probably not, but all of the other tests have the same prefix.

Feel free to send patches fixing old mistakes, too.

> > > +	return [check_no_compiler_messages powerpc_prefixed_addr_ok object {

> > Does this need to be separate from check_effective_target_powerpc_future_ok
> > at all?
> 
> This gets back to whether some port will eventually want to use FUTURE
> instructions in a 32-bit context.  Linux will always be 64-bit little endian,

No, Linux also supports 32-bit BE and 64-bit BE.  Even if we do not
support Future on those currently, this may change.  There is absolutely
no reason to make it harder to do later.  If we think some combinations
do not exist we can *ignore* them.  That is much less work, too.

> but there are other platforms out there that may want to generate FUTURE code.

But we should not anticipate those having broken assemblers that do not
support half of the insns, etc.  You *already* required prefixed insns in
the base Future test, to prove how futile this is.

> > > +# support PC-relative addressing when -mcpu=future is used to pass this test.
> > > +
> > > +proc check_effective_target_powerpc_pcrel_ok { } {
> > > +    if { ([istarget powerpc*-*-*]) } {
> > > +	return [check_no_compiler_messages powerpc_pcrel_ok object {
> > > +	      int main (void) {
> > > +		  static int s __attribute__((__used__));
> > > +		  int e;
> > > +		  asm ("plwa %0,s@pcrel(0),1" : "=r" (e));
> > > +		  return e;
> > > +	      }
> > > +	  } "-mfuture"]
> > > +      } else {
> > > +	  return 0
> > > +      }
> > > +}
> > 
> > So every assembler will support either all three of these, or none.
> > Can you simplify this please?
> 
> I can imagine for example AIX might support 64-bit 'future' but not support
> PC-relative.  I believe you (or David) asked to split up the prefixed
> addressing with numeric offets and PC-relative addressing, because ports might
> not be able to support PC-relative addressing.

That is fine, but these tests do not do that.  Some clearer comments will
help, too.


Segher

Re: [PATCH], V7, #6 of 7, Fix issues with vector extract and prefixed instructions

[Segher Boessenkool]

On Tue, Dec 03, 2019 at 01:20:04PM -0500, Michael Meissner wrote:
> On Tue, Nov 26, 2019 at 01:20:20PM -0600, Segher Boessenkool wrote:
> > > I needed to add a new constraint (em) in addition to new predicate functions.
> > > I discovered that with the predicate function alone, the register allocator
> > > would re-create the address.  The constraint prevents this combination.
> > 
> > It's a reasonable thing to have as a contraint, too.
> > 
> > Once again, how should things work in inline assembler?  Can we allow
> > prefixed addressing there, or should "m" in inline asm mean "em"?
> 
> At the moment, I think we should just not allow prefixed addresses in asm
> constructs at all.

We need to think about this *before GCC 10*.  And have patches for it.

So you are saying that yes, "m" in inline asm should mean "em".  This
probably makes sense, there is so much existing asm that does not work
with prefixed addresses -- this is somewhet similar to how "m" in inline
asm is not the same as "m<>" (as it is in the machine description).

But then we need an extra constraint to _do_ allow prefixed addresses:
there needs to be a way to write inline asm for such memory as well.

> > > I also modified the vector extract code to generate a single PC-relative load
> > > if the vector has a PC-relative address and the offset is constant.
> > 
> > That should be handled by the RTL optimisers anyway, no?  Or is this
> > for post-reload splitters (a bad idea always).
> 
> You have it backwards.  It is the RTL optimizers that combines the vector
> extract with the load in the first place.  My code allows the combination and
> generates a single instruction.  Otherwise, it would do a PLA and then do the
> vector extract with the address loaded.

So, separate patch please, for a separate improvement.  So that I can
understand what is what.

> > > +  /* Optimize PC-relative addresses with a constant offset.  */
> > > +  else if (pcrel_p && CONST_INT_P (element_offset))
> > > +    {
> > > +      rtx addr2 = addr;
> > 
> > addr isn't used after this anyway, so you can clobber it just fine.
> 
> Generally I prefer not to reuse variables.

You shouldn't fear that.  Instead, you should factor big routines, and/or
simplify control flow.

"Not reusing variables" (for the *same purpose* btw) is not the problem.
The problem is that the code is too big and complex and irregular to
simply understand.

> > > +  /* With only one temporary base register, we can't support a PC-relative
> > > +     address added to a variable offset.  This is because the PADDI instruction
> > > +     requires RA to be 0 when doing a PC-relative add (i.e. no register to add
> > > +     to).  */
> > > +  else if (pcrel_p)
> > > +    gcc_unreachable ();
> > 
> > That comment suggests we just ICE when we get unwanted input.  Such a
> > comment belongs where we prevent such code from being formed in the
> > first place (or nowhere, if it is obvious).
> 
> The constraint and predicate is where we prevent it from occuring.  The
> gcc_unreachable is just there as insurance that it didn't get recreated later.
> During testing before I added the constraint, I found the register allocator
> combining the two, even though the predicate didn't allow the combination.  So
> the test is just to ICE out if the combination took place.

So there should be no comment here?  A gcc_assert could be useful, but
that is all?  And the assert should be as early as possible, in general,
like (almost) immediately after the function start.  If you find you want
to do it later, congratulations, you found a good place to factor this
routine :-)


Segher