Add support for PowerPC prefixed memory instructions and pc-relative support (Intro)

Add support for PowerPC prefixed memory instructions and pc-relative support (Intro)

[Michael Meissner]

To keep things organized, I'm going to start submitting the patches for for a
possible future PowerPC machine's prefixed addressing (including pc-relative
suport) as threads under this message.

There are two patches that I've already submitted that are needed for the rest
of the patches:

Patch #1
PR Fix floatsi{sf,df}2 and floatunssi{sf,df}2 for a future powerpc machine
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01791.html

Patch #2
Prepare for prefixed instructions on PowerPC
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01812.html

I am trying to break the patches into smaller related pieces, instead of just
submitting a combination patch.

-- 
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] PowerPC Prefixed Memory, Patch #3, Update predicates

[Michael Meissner]

This patch updates the predicates for prefixed addressing.

This patch deletes a predicate that I had originally added, but the code no
longer uses.

This patch changes how local symbols for pc-relative addressing are marked.
Previously, we had used a machine dependent bit in the SYMBOL_REF node.  Now, I
use the SYMBOL_REF_LOCAL_P bit.

This patch adds a predicate for handling external addresses that will be used
for future patches.

I have done bootstrap builds and make check.  There were no regressions.  Can I
check this patch into the trunk?

2019-06-27  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (pcrel_address):  Use
	SYMBOL_REF_LOCAL_P to determine if a label is local.
	(pcrel_external_address): New predicate.
	(non_prefixed_mem_operand): Delete, predicate not used.
	* config/rs6000/rs6000.h (SYMBOL_FLAG_PCREL_P): Delete, we now use
	SYMBOL_REF_LOCAL_P to determine if we can use pc-relative
	addressing.
	(SYMBOL_REF_PCREL_P): Likewise.

Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 272766)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -1645,21 +1645,57 @@ (define_predicate "pcrel_address"
       op = op0;
     }
 
-  return LABEL_REF_P (op) || SYMBOL_REF_PCREL_P (op);
+  if (LABEL_REF_P (op))
+    return true;
+
+  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op)
+	  && SYMBOL_REF_LOCAL_P (op));
 })
 
-;; Return 1 if op is a prefixed memory operand
+;; Return true if the operand is an external symbol whose address can be loaded
+;; into a register either via PADDI (if the symbol is in the main program) or
+;; PLD GOT address (if the symbol is defined in a shared library).
+
+(define_predicate "pcrel_external_address"
+  (match_code "symbol_ref,const")
+{
+  if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM)
+    return false;
+
+  /* Discard any CONST's.  */
+  if (GET_CODE (op) == CONST)
+    op = XEXP (op, 0);
+
+  /* Validate offset.  */
+  if (GET_CODE (op) == PLUS)
+    {
+      rtx op0 = XEXP (op, 0);
+      rtx op1 = XEXP (op, 1);
+
+      if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1)))
+	return false;
+
+      op = op0;
+    }
+
+  return (SYMBOL_REF_P (op) && !SYMBOL_REF_LOCAL_P (op));
+})
+
+;; Return 1 if op is a prefixed memory operand.
 (define_predicate "prefixed_mem_operand"
   (match_code "mem")
 {
   return rs6000_prefixed_address (XEXP (op, 0), GET_MODE (op));
 })
 
-;; Return 1 if op is a memory operand that is not a prefixed memory
-;; operand.
-(define_predicate "non_prefixed_mem_operand"
-  (and (match_operand 0 "memory_operand")
-       (not (match_operand 0 "prefixed_mem_operand"))))
+;; Return 1 if op is a memory operand to an external variable when we
+;; support pc-relative addressing and the PCREL_OPT relocation to
+;; optimize references to it.
+(define_predicate "pcrel_external_mem_operand"
+  (match_code "mem")
+{
+  return pcrel_external_address (XEXP (op, 0), Pmode);
+})
 
 ;; Match the first insn (addis) in fusing the combination of addis and loads to
 ;; GPR registers on power8.
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 272766)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -2550,10 +2550,3 @@ typedef struct GTY(()) machine_function
   IN_RANGE ((VALUE),							\
 	    -(HOST_WIDE_INT_1 << 33),					\
 	    (HOST_WIDE_INT_1 << 33) - 1 - (EXTRA))
-
-/* Flag to mark SYMBOL_REF objects to say they are local addresses and are used
-   in pc-relative addresses.  */
-#define SYMBOL_FLAG_PCREL	SYMBOL_FLAG_MACH_DEP
-
-#define SYMBOL_REF_PCREL_P(X)						\
-  (SYMBOL_REF_P (X) && SYMBOL_REF_FLAGS (X) & SYMBOL_FLAG_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] PowerPC Prefixed Memory, Patch #3, Update predicates

[Segher Boessenkool]

Hi Mike,

On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote:
> +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op)
> +	  && SYMBOL_REF_LOCAL_P (op));

Please break the line before that first && as well?

> +(define_predicate "pcrel_external_address"
> +  (match_code "symbol_ref,const")
> +{
> +  if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM)
> +    return false;

  if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM))

Should there be a helper function for this?  PCREL is only supported
for medium model -- abstracting that makes both the current code easier
to read, and if we ever want to support other models, that will be
simpler to do as well.

> +  /* Discard any CONST's.  */
> +  if (GET_CODE (op) == CONST)
> +    op = XEXP (op, 0);

That comment says nothing the code already tells.  It's a common thing
to do anyway; just don't comment on it?

> +;; Return 1 if op is a memory operand to an external variable when we
> +;; support pc-relative addressing and the PCREL_OPT relocation to
> +;; optimize references to it.
> +(define_predicate "pcrel_external_mem_operand"
> +  (match_code "mem")
> +{
> +  return pcrel_external_address (XEXP (op, 0), Pmode);
> +})

Is that comment correct?  pcrel_external_address does nothing with
PCREL_OPT?  Or _its_ comment doesn't say, at least.

Okay for trunk with those trivialities resolved.  Thanks!


Segher

Re: [PATCH] PowerPC Prefixed Memory, Patch #3, Update predicates

[Bill Schmidt]

On 6/28/19 8:20 AM, Segher Boessenkool wrote:
> Hi Mike,
>
> On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote:
>> +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op)
>> +	  && SYMBOL_REF_LOCAL_P (op));
> Please break the line before that first && as well?
>
>> +(define_predicate "pcrel_external_address"
>> +  (match_code "symbol_ref,const")
>> +{
>> +  if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM)
>> +    return false;
>   if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM))
>
> Should there be a helper function for this?  PCREL is only supported
> for medium model -- abstracting that makes both the current code easier
> to read, and if we ever want to support other models, that will be
> simpler to do as well.

I'd suggest

    if (!rs6000_pcrel_p ())

which will also make sure this is ELFv2.

Thanks,
Bill
>
>> +  /* Discard any CONST's.  */
>> +  if (GET_CODE (op) == CONST)
>> +    op = XEXP (op, 0);
> That comment says nothing the code already tells.  It's a common thing
> to do anyway; just don't comment on it?
>
>> +;; Return 1 if op is a memory operand to an external variable when we
>> +;; support pc-relative addressing and the PCREL_OPT relocation to
>> +;; optimize references to it.
>> +(define_predicate "pcrel_external_mem_operand"
>> +  (match_code "mem")
>> +{
>> +  return pcrel_external_address (XEXP (op, 0), Pmode);
>> +})
> Is that comment correct?  pcrel_external_address does nothing with
> PCREL_OPT?  Or _its_ comment doesn't say, at least.
>
> Okay for trunk with those trivialities resolved.  Thanks!
>
>
> Segher
>

Re: [PATCH] PowerPC Prefixed Memory, Patch #3, Update predicates

[Michael Meissner]

On Fri, Jun 28, 2019 at 08:20:35AM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Thu, Jun 27, 2019 at 08:12:35PM -0400, Michael Meissner wrote:
> > +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_P (op)
> > +	  && SYMBOL_REF_LOCAL_P (op));
> 
> Please break the line before that first && as well?

Ok.

> > +(define_predicate "pcrel_external_address"
> > +  (match_code "symbol_ref,const")
> > +{
> > +  if (!TARGET_PCREL || TARGET_CMODEL != CMODEL_MEDIUM)
> > +    return false;
> 
>   if (!(TARGET_PCREL && TARGET_CMODEL == CMODEL_MEDIUM))
> 
> Should there be a helper function for this?  PCREL is only supported
> for medium model -- abstracting that makes both the current code easier
> to read, and if we ever want to support other models, that will be
> simpler to do as well.

Yeah, Bill's suggestion is probably what I should use.

> > +  /* Discard any CONST's.  */
> > +  if (GET_CODE (op) == CONST)
> > +    op = XEXP (op, 0);
> 
> That comment says nothing the code already tells.  It's a common thing
> to do anyway; just don't comment on it?

Ok.

> > +;; Return 1 if op is a memory operand to an external variable when we
> > +;; support pc-relative addressing and the PCREL_OPT relocation to
> > +;; optimize references to it.
> > +(define_predicate "pcrel_external_mem_operand"
> > +  (match_code "mem")
> > +{
> > +  return pcrel_external_address (XEXP (op, 0), Pmode);
> > +})
> 
> Is that comment correct?  pcrel_external_address does nothing with
> PCREL_OPT?  Or _its_ comment doesn't say, at least.

Yes, I will re-word it.  We will need this predicate even if PCREL_OPT is not
being used.

> Okay for trunk with those trivialities resolved.  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

[PATCH] PowerPC Prefixed Memory, Patch #4, Add pc-relative reference support

[Michael Meissner]

At the moment, we have two functions that create and look at TOC references:

	create_TOC_reference	(global function)
	use_toc_relative_ref	(static function in rs6000.c)

Since I am adding pc-relative support that will be used instead of TOC support,
this patch renames these two functions to be:

	create_data_reference
	use_toc_or_pc_relative_ref

and it adds some of the foundation for adding pc-relative support.  Note, it
will need future patches to completely flesh out the pc-relative support.  As
per a previous private discussion, I kept the old create_TOC_reference and
added a gcc assert if it is called when pc-relative addressing is used.  There
is currently one place that still calls create_TOC_reference (in a section
using TLS addresses).

I have done a bootstrap on a power8 little endian Linux system with no
regressions.  Can I check this into the trunk?

2019-06-28  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-logue.c (create_TOC_reference): Move debug
	statements to create_data_reference.  Add gcc_assert to make sure
	we are using TOCs and not pc-relative addressing.
	(create_data_reference): New function, create either a TOC style
	reference or a pc-relative reference to local symbols.
	* config/rs6000/rs6000-protos.h (create_data_reference): Add
	declaration.
	* config/rs6000/rs6000.c (rs6000_legitimize_address): Call
	create_data_reference instead of create_TOC_reference.
	(use_toc_or_pc_relative_ref): Rename from use_toc_relative_ref and
	add support for pc-relative addressing.
	(rs6000_emit_move): Call use_toc_or_pc_relative_ref and
	create_data_reference instead of use_toc_relative_ref and
	create_TOC_reference.
	* config/rs6000/rs6000.md (cmp<mode>_internal2): Call
	create_data_reference instead of create_TOC_reference.

Index: gcc/config/rs6000/rs6000-logue.c
===================================================================
--- gcc/config/rs6000/rs6000-logue.c	(revision 272714)
+++ gcc/config/rs6000/rs6000-logue.c	(working copy)
@@ -1406,23 +1406,13 @@ uses_TOC (void)
 }
 #endif
 
+/* Create a TOC style reference for a symbol.  */
 rtx
 create_TOC_reference (rtx symbol, rtx largetoc_reg)
 {
   rtx tocrel, tocreg, hi;
 
-  if (TARGET_DEBUG_ADDR)
-    {
-      if (SYMBOL_REF_P (symbol))
-	fprintf (stderr, "\ncreate_TOC_reference, (symbol_ref %s)\n",
-		 XSTR (symbol, 0));
-      else
-	{
-	  fprintf (stderr, "\ncreate_TOC_reference, code %s:\n",
-		   GET_RTX_NAME (GET_CODE (symbol)));
-	  debug_rtx (symbol);
-	}
-    }
+  gcc_assert (TARGET_TOC && !TARGET_PCREL);
 
   if (!can_create_pseudo_p ())
     df_set_regs_ever_live (TOC_REGISTER, true);
@@ -1441,6 +1431,39 @@ create_TOC_reference (rtx symbol, rtx la
   return gen_rtx_LO_SUM (Pmode, hi, tocrel);
 }
 
+/* Create either a TOC reference to a locally defined item or a pc-relative
+   reference, depending on the ABI.  */
+rtx
+create_data_reference (rtx symbol, rtx largetoc_reg)
+{
+  if (TARGET_DEBUG_ADDR)
+    {
+      const char *toc_or_pcrel = (TARGET_PCREL) ? "pc-relative" : "TOC";
+
+      if (SYMBOL_REF_P (symbol))
+	fprintf (stderr, "\ncreate_data_reference, abi %s, (symbol_ref %s)\n",
+		 toc_or_pcrel, XSTR (symbol, 0));
+      else
+	{
+	  fprintf (stderr, "\ncreate_data_reference, abi %s, code %s:\n",
+		   toc_or_pcrel, GET_RTX_NAME (GET_CODE (symbol)));
+	  debug_rtx (symbol);
+	}
+    }
+
+  /* We don't have to do anything special for pc-relative references.  The
+     SYMBOL_REF bits sayss whether the label is local where we can use
+     pc-relative addressing, or if we have to load the address from the GOT
+     section to use it on external addresses.  */
+  if (TARGET_PCREL)
+    {
+      gcc_assert (TARGET_CMODEL == CMODEL_MEDIUM);
+      return symbol;
+    }
+
+  return create_TOC_reference (symbol, largetoc_reg);
+}
+
 /* Issue assembly directives that create a reference to the given DWARF
    FRAME_TABLE_LABEL from the current function section.  */
 void
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 272714)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -131,6 +131,7 @@ extern void rs6000_emit_swdiv (rtx, rtx,
 extern void rs6000_emit_swsqrt (rtx, rtx, bool);
 extern void output_toc (FILE *, rtx, int, machine_mode);
 extern void rs6000_fatal_bad_address (rtx);
+extern rtx create_data_reference (rtx, rtx);
 extern rtx create_TOC_reference (rtx, rtx);
 extern void rs6000_split_multireg_move (rtx, rtx);
 extern void rs6000_emit_le_vsx_permute (rtx, rtx, machine_mode);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 272766)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8084,7 +8084,7 @@ rs6000_legitimize_address (rtx x, rtx ol
 	   && SYMBOL_REF_P (x)
 	   && constant_pool_expr_p (x)
 	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (x), Pmode))
-    return create_TOC_reference (x, NULL_RTX);
+    return create_data_reference (x, NULL_RTX);
   else
     return x;
 }
@@ -8696,19 +8696,21 @@ rs6000_cannot_force_const_mem (machine_m
   return TARGET_ELF && tls_referenced_p (x);
 }
 
-/* Return true iff the given SYMBOL_REF refers to a constant pool entry
-   that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
-   can be addressed relative to the toc pointer.  */
+/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
+   have put in the TOC or is referenced via a pc-relative reference.  In
+   addition, for cmodel=medium, if the SYMBOL_REF can be addressed relative to
+   the either toc pointer or via a pc-relative reference.  */
 
 static bool
-use_toc_relative_ref (rtx sym, machine_mode mode)
+use_toc_or_pc_relative_ref (rtx sym, machine_mode mode)
 {
   return ((constant_pool_expr_p (sym)
 	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
 					       get_pool_mode (sym)))
 	  || (TARGET_CMODEL == CMODEL_MEDIUM
 	      && SYMBOL_REF_LOCAL_P (sym)
-	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
+	      && (TARGET_PCREL
+		  || GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT)));
 }
 
 /* TARGET_LEGITIMATE_ADDRESS_P recognizes an RTL expression
@@ -9810,8 +9812,8 @@ rs6000_emit_move (rtx dest, rtx source,
 	 reference to it.  */
       if (TARGET_TOC
 	  && SYMBOL_REF_P (operands[1])
-	  && use_toc_relative_ref (operands[1], mode))
-	operands[1] = create_TOC_reference (operands[1], operands[0]);
+	  && use_toc_or_pc_relative_ref (operands[1], mode))
+	operands[1] = create_data_reference (operands[1], operands[0]);
       else if (mode == Pmode
 	       && CONSTANT_P (operands[1])
 	       && GET_CODE (operands[1]) != HIGH
@@ -9864,11 +9866,11 @@ rs6000_emit_move (rtx dest, rtx source,
 
 	  if (TARGET_TOC
 	      && SYMBOL_REF_P (XEXP (operands[1], 0))
-	      && use_toc_relative_ref (XEXP (operands[1], 0), mode))
+	      && use_toc_or_pc_relative_ref (XEXP (operands[1], 0), mode))
 	    {
-	      rtx tocref = create_TOC_reference (XEXP (operands[1], 0),
-						 operands[0]);
-	      operands[1] = gen_const_mem (mode, tocref);
+	      rtx dataref = create_data_reference (XEXP (operands[1], 0),
+						   operands[0]);
+	      operands[1] = gen_const_mem (mode, dataref);
 	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
 	    }
 	}
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 272757)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11666,9 +11666,9 @@ (define_insn_and_split "*cmp<mode>_inter
   if (TARGET_TOC)
     {
       rtx tocref;
-      tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]);
+      tocref = create_data_reference (XEXP (operands[14], 0), operands[11]);
       operands[14] = gen_const_mem (DFmode, tocref);
-      tocref = create_TOC_reference (XEXP (operands[15], 0), operands[11]);
+      tocref = create_data_reference (XEXP (operands[15], 0), operands[11]);
       operands[15] = gen_const_mem (DFmode, tocref);
       set_mem_alias_set (operands[14], get_TOC_alias_set ());
       set_mem_alias_set (operands[15], get_TOC_alias_set ());

-- 
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] PowerPC Prefixed Memory, Patch #4, Add pc-relative reference support

[Segher Boessenkool]

Hi Mike,

On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote:
> --- gcc/config/rs6000/rs6000-logue.c	(revision 272714)
> +++ gcc/config/rs6000/rs6000-logue.c	(working copy)
> @@ -1406,23 +1406,13 @@ uses_TOC (void)
>  }
>  #endif
>  
> +/* Create a TOC style reference for a symbol.  */
>  rtx
>  create_TOC_reference (rtx symbol, rtx largetoc_reg)

Does this really belong in this file?  It doesn't do anythin *logue, and
it isn't called from anywhere in here.

> +/* Create either a TOC reference to a locally defined item or a pc-relative
> +   reference, depending on the ABI.  */
> +rtx
> +create_data_reference (rtx symbol, rtx largetoc_reg)

Same here.

What is largetoc_reg?  The function comment should say.  It also is only
relevant for create_TOC_reference (where such a comment is also missing),
so could you factor this better please?

Probably a create_data_reference with only one argument?  Which calls
create_TOC_reference with a NULL second arg.  It looks like your
proposed create_data_reference will not do the right thing if called
with a non-null second arg if pcrel.  Perhaps that cannot happen, but
make that clear then?  Just an assert will do, bigger cleanups are
better of course.


Segher

Re: [PATCH] PowerPC Prefixed Memory, Patch #4, Add pc-relative reference support

[Michael Meissner]

On Wed, Jul 03, 2019 at 05:09:57PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote:
> > --- gcc/config/rs6000/rs6000-logue.c	(revision 272714)
> > +++ gcc/config/rs6000/rs6000-logue.c	(working copy)
> > @@ -1406,23 +1406,13 @@ uses_TOC (void)
> >  }
> >  #endif
> >  
> > +/* Create a TOC style reference for a symbol.  */
> >  rtx
> >  create_TOC_reference (rtx symbol, rtx largetoc_reg)
> 
> Does this really belong in this file?  It doesn't do anythin *logue, and
> it isn't called from anywhere in here.

Note, when rs6000-logue.c was created, it was put there.  I was just trying to
make the smallest number of changes to the infrastructure.  I can move it back
to rs6000.c if preferred.

> > +/* Create either a TOC reference to a locally defined item or a pc-relative
> > +   reference, depending on the ABI.  */
> > +rtx
> > +create_data_reference (rtx symbol, rtx largetoc_reg)
> 
> Same here.
> 
> What is largetoc_reg?  The function comment should say.  It also is only
> relevant for create_TOC_reference (where such a comment is also missing),
> so could you factor this better please?

Again, this was existing code, and I didn't really change the existing code.

> Probably a create_data_reference with only one argument?  Which calls
> create_TOC_reference with a NULL second arg.  It looks like your
> proposed create_data_reference will not do the right thing if called
> with a non-null second arg if pcrel.  Perhaps that cannot happen, but
> make that clear then?  Just an assert will do, bigger cleanups are
> better of course.

-- 
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] PowerPC Prefixed Memory, Patch #4, Add pc-relative reference support

[Segher Boessenkool]

On Mon, Jul 08, 2019 at 02:42:13PM -0400, Michael Meissner wrote:
> On Wed, Jul 03, 2019 at 05:09:57PM -0500, Segher Boessenkool wrote:
> > Hi Mike,
> > 
> > On Fri, Jun 28, 2019 at 02:50:33PM -0400, Michael Meissner wrote:
> > > --- gcc/config/rs6000/rs6000-logue.c	(revision 272714)
> > > +++ gcc/config/rs6000/rs6000-logue.c	(working copy)
> > > @@ -1406,23 +1406,13 @@ uses_TOC (void)
> > >  }
> > >  #endif
> > >  
> > > +/* Create a TOC style reference for a symbol.  */
> > >  rtx
> > >  create_TOC_reference (rtx symbol, rtx largetoc_reg)
> > 
> > Does this really belong in this file?  It doesn't do anythin *logue, and
> > it isn't called from anywhere in here.
> 
> Note, when rs6000-logue.c was created, it was put there.

I know.  That doesn't make it right though!  :-)

> I was just trying to
> make the smallest number of changes to the infrastructure.  I can move it back
> to rs6000.c if preferred.

Please do; as a separate patch.  Thanks in advance.  A patch purely moving
it back is pre-approved.

> > > +/* Create either a TOC reference to a locally defined item or a pc-relative
> > > +   reference, depending on the ABI.  */
> > > +rtx
> > > +create_data_reference (rtx symbol, rtx largetoc_reg)
> > 
> > Same here.
> > 
> > What is largetoc_reg?  The function comment should say.  It also is only
> > relevant for create_TOC_reference (where such a comment is also missing),
> > so could you factor this better please?
> 
> Again, this was existing code, and I didn't really change the existing code.

No, create_data_reference is a new function.  That's the point.  For
create_TOC_reference it might make some sense (but it should be documented
what this does); for create_data_reference it is a non-sensical parameter:
either all callers use NULL, or you more likely than not have bugs.

> > Probably a create_data_reference with only one argument?  Which calls
> > create_TOC_reference with a NULL second arg.  It looks like your
> > proposed create_data_reference will not do the right thing if called
> > with a non-null second arg if pcrel.  Perhaps that cannot happen, but
> > make that clear then?  Just an assert will do, bigger cleanups are
> > better of course.


Segher

[PATCH, committed] PowerPC Prefixed Memory, Patch #5 (move create_TOC_reference)

[Michael Meissner]

On Mon, Jul 08, 2019 at 01:53:13PM -0500, Segher Boessenkool wrote:
> Please do; as a separate patch.  Thanks in advance.  A patch purely moving
> it back is pre-approved.

I just committed this patch:

2019-07-09  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-internal.h (create_TOC_reference): Delete.
	* config/rs6000/rs6000-logue.c (create_TOC_reference): Move
	function from rs6000-logue.c back to rs6000.c.
	* config/rs6000/rs6000.c (create_TOC_reference): Likewise.

Index: gcc/config/rs6000/rs6000-internal.h
===================================================================
--- gcc/config/rs6000/rs6000-internal.h	(revision 273308)
+++ gcc/config/rs6000/rs6000-internal.h	(working copy)
@@ -92,7 +92,6 @@ extern void rs6000_emit_prologue_compone
 extern void rs6000_emit_epilogue_components (sbitmap components);
 extern void rs6000_set_handled_components (sbitmap components);
 extern rs6000_stack_t * rs6000_stack_info (void);
-extern rtx create_TOC_reference (rtx symbol, rtx largetoc_reg);
 extern rtx rs6000_got_sym (void);
 extern struct machine_function *rs6000_init_machine_status (void);
 extern bool save_reg_p (int reg);
Index: gcc/config/rs6000/rs6000-logue.c
===================================================================
--- gcc/config/rs6000/rs6000-logue.c	(revision 273308)
+++ gcc/config/rs6000/rs6000-logue.c	(working copy)
@@ -1406,41 +1406,6 @@ uses_TOC (void)
 }
 #endif
 
-rtx
-create_TOC_reference (rtx symbol, rtx largetoc_reg)
-{
-  rtx tocrel, tocreg, hi;
-
-  if (TARGET_DEBUG_ADDR)
-    {
-      if (SYMBOL_REF_P (symbol))
-	fprintf (stderr, "\ncreate_TOC_reference, (symbol_ref %s)\n",
-		 XSTR (symbol, 0));
-      else
-	{
-	  fprintf (stderr, "\ncreate_TOC_reference, code %s:\n",
-		   GET_RTX_NAME (GET_CODE (symbol)));
-	  debug_rtx (symbol);
-	}
-    }
-
-  if (!can_create_pseudo_p ())
-    df_set_regs_ever_live (TOC_REGISTER, true);
-
-  tocreg = gen_rtx_REG (Pmode, TOC_REGISTER);
-  tocrel = gen_rtx_UNSPEC (Pmode, gen_rtvec (2, symbol, tocreg), UNSPEC_TOCREL);
-  if (TARGET_CMODEL == CMODEL_SMALL || can_create_pseudo_p ())
-    return tocrel;
-
-  hi = gen_rtx_HIGH (Pmode, copy_rtx (tocrel));
-  if (largetoc_reg != NULL)
-    {
-      emit_move_insn (largetoc_reg, hi);
-      hi = largetoc_reg;
-    }
-  return gen_rtx_LO_SUM (Pmode, hi, tocrel);
-}
-
 /* Issue assembly directives that create a reference to the given DWARF
    FRAME_TABLE_LABEL from the current function section.  */
 void
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273308)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -7735,6 +7735,45 @@ constant_pool_expr_p (rtx op)
 	  && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (base), Pmode));
 }
 
+/* Create a TOC reference for symbol_ref SYMBOL.  If LARGETOC_REG is non-null,
+   use that as the register to put the HIGH value into if register allocation
+   is already done.  */
+
+rtx
+create_TOC_reference (rtx symbol, rtx largetoc_reg)
+{
+  rtx tocrel, tocreg, hi;
+
+  if (TARGET_DEBUG_ADDR)
+    {
+      if (SYMBOL_REF_P (symbol))
+	fprintf (stderr, "\ncreate_TOC_reference, (symbol_ref %s)\n",
+		 XSTR (symbol, 0));
+      else
+	{
+	  fprintf (stderr, "\ncreate_TOC_reference, code %s:\n",
+		   GET_RTX_NAME (GET_CODE (symbol)));
+	  debug_rtx (symbol);
+	}
+    }
+
+  if (!can_create_pseudo_p ())
+    df_set_regs_ever_live (TOC_REGISTER, true);
+
+  tocreg = gen_rtx_REG (Pmode, TOC_REGISTER);
+  tocrel = gen_rtx_UNSPEC (Pmode, gen_rtvec (2, symbol, tocreg), UNSPEC_TOCREL);
+  if (TARGET_CMODEL == CMODEL_SMALL || can_create_pseudo_p ())
+    return tocrel;
+
+  hi = gen_rtx_HIGH (Pmode, copy_rtx (tocrel));
+  if (largetoc_reg != NULL)
+    {
+      emit_move_insn (largetoc_reg, hi);
+      hi = largetoc_reg;
+    }
+  return gen_rtx_LO_SUM (Pmode, hi, tocrel);
+}
+
 /* These are only used to pass through from print_operand/print_operand_address
    to rs6000_output_addr_const_extra over the intervening function
    output_addr_const which is not target code.  */

-- 
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, committed] PowerPC Prefixed Memory, Patch #5 (move create_TOC_reference)

[Segher Boessenkool]

On Tue, Jul 09, 2019 at 01:32:26PM -0400, Michael Meissner wrote:
> On Mon, Jul 08, 2019 at 01:53:13PM -0500, Segher Boessenkool wrote:
> > Please do; as a separate patch.  Thanks in advance.  A patch purely moving
> > it back is pre-approved.
> 
> I just committed this patch:
> 
> 2019-07-09  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/rs6000-internal.h (create_TOC_reference): Delete.
> 	* config/rs6000/rs6000-logue.c (create_TOC_reference): Move
> 	function from rs6000-logue.c back to rs6000.c.
> 	* config/rs6000/rs6000.c (create_TOC_reference): Likewise.

Thanks!


Segher

[PATCH], PowerPC, Patch #6, Create pc-relative addressing insns

[Michael Meissner]

This patch updates the basic support for pc-relative addressing that will be
added in a future machine.

It was originally proposed as patch #4:
https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01866.html

Segher suggested that I split out moving create_TOC_reference back to rs6000.c
as a separate patch, which I did in this patch that has been committed:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00719.html

In working on the other suggestions from Segher about the second argument to
create_TOC_reference (which is where to put the HIGH value after register
allocation), I came to the conclusion it wasn't worth it to try and combine TOC
and pc-relative addressing.  There are two places in rs6000_emit_move that deal
with the creation of TOC/pc-relative addressing.

There is also one place in rs6000.md that created TOC addressing that needed to
support pc-relative addressing also.  It does the compare of two __ibm128
floating point values when the -mcompat-xl switch is used, and the comparison
needs to compare the values against 0 and +infinity, and it needs to load up
the two constants.

Instead of trying to combine the two addressing forms, I added separate support
for pc-relative addressing, and added extra checks in supporting TOC
addressing.

The one place where I combined TOC and pc-relative addressing is using a common
function to set the memory alias set group.  Originally it was called
get_TOC_alias_set, and I renamed it to get_data_alias_set and I changed all of
the callers.

There will be several other patches before all of the pc-relative support is
available and turned on by default.

I have bootstraped the compiler on a little endian power8 system and there were
no regressions in the test suite.  Can I check it into the trunk?

2019-07-09  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-protos.h (get_data_alias_set): Rename
	get_TOC_alias_set for use with both TOC & pc-relative addressing.
	* config/rs6000/rs6000.c (create_TOC_reference): Add gcc_assert.
	(rs6000_legitimize_address): Add check for not pc-relative in TOC
	support.
	(rs6000_legitimize_tls_address_aix): Call get_data_alias_set
	instead of get_TOC_alias_set.
	(use_toc_relative_ref): Add check for SYMBOL_REF and not
	pc-relative to simplify callers.  Make tests easier to
	understand.
	(use_pc_relative_ref): New helper function.
	(rs6000_emit_move): Add basic support for pc-relative addressing.
	Call get_data_alias_set instead of get_TOC_alias_set.
	(data_alias_set): Rename static variable.
	(get_data_alias_set): Rename get_TOC_alias_set since it is now
	used for both TOC and pc-relative addressing.
	* config/rs6000/rs6000.md (cmp<mode>_internal2): Add support for
	pc-relative addressing.  Call get_data_alias_set instead of
	get_TOC_alias_set.

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273255)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -189,7 +189,7 @@ extern void rs6000_gen_section_name (cha
 extern void output_function_profiler (FILE *, int);
 extern void output_profile_hook  (int);
 extern int rs6000_trampoline_size (void);
-extern alias_set_type get_TOC_alias_set (void);
+extern alias_set_type get_data_alias_set (void);
 extern void rs6000_emit_prologue (void);
 extern void rs6000_emit_load_toc_table (int);
 extern unsigned int rs6000_dbx_register_number (unsigned int, unsigned int);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273310)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -7740,6 +7740,8 @@ create_TOC_reference (rtx symbol, rtx la
 {
   rtx tocrel, tocreg, hi;
 
+  gcc_assert (TARGET_TOC && !TARGET_PCREL);
+
   if (TARGET_DEBUG_ADDR)
     {
       if (SYMBOL_REF_P (symbol))
@@ -8137,7 +8139,7 @@ rs6000_legitimize_address (rtx x, rtx ol
 	emit_insn (gen_macho_high (reg, x));
       return gen_rtx_LO_SUM (Pmode, reg, x);
     }
-  else if (TARGET_TOC
+  else if (TARGET_TOC && !TARGET_PCREL
 	   && SYMBOL_REF_P (x)
 	   && constant_pool_expr_p (x)
 	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (x), Pmode))
@@ -8441,7 +8443,7 @@ rs6000_legitimize_tls_address_aix (rtx a
     {
       tocref = create_TOC_reference (XEXP (sym, 0), NULL_RTX);
       mem = gen_const_mem (Pmode, tocref);
-      set_mem_alias_set (mem, get_TOC_alias_set ());
+      set_mem_alias_set (mem, get_data_alias_set ());
     }
   else
     return sym;
@@ -8459,7 +8461,7 @@ rs6000_legitimize_tls_address_aix (rtx a
       SYMBOL_REF_FLAGS (modaddr) |= SYMBOL_FLAG_LOCAL;
       tocref = create_TOC_reference (modaddr, NULL_RTX);
       rtx modmem = gen_const_mem (Pmode, tocref);
-      set_mem_alias_set (modmem, get_TOC_alias_set ());
+      set_mem_alias_set (modmem, get_data_alias_set ());
       
       rtx modreg = gen_reg_rtx (Pmode);
       emit_insn (gen_rtx_SET (modreg, modmem));
@@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
 static bool
 use_toc_relative_ref (rtx sym, machine_mode mode)
 {
-  return ((constant_pool_expr_p (sym)
-	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
-					       get_pool_mode (sym)))
-	  || (TARGET_CMODEL == CMODEL_MEDIUM
-	      && SYMBOL_REF_LOCAL_P (sym)
-	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
+  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
+    return false;
+
+  if (constant_pool_expr_p (sym)
+      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
+					  get_pool_mode (sym)))
+    return true;
+
+  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
+	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
+}
+
+/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
+   have put in the pc-relative constant area, or for cmodel=medium, if the
+   SYMBOL_REF can be addressed via pc-relative instructions.  */
+
+static bool
+use_pc_relative_ref (rtx sym)
+{
+  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
+    return false;
+
+  if (constant_pool_expr_p (sym)
+      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
+					  get_pool_mode (sym)))
+    return true;
+
+  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym));
 }
 
 /* TARGET_LEGITIMATE_ADDRESS_P recognizes an RTL expression
@@ -9865,10 +9889,14 @@ rs6000_emit_move (rtx dest, rtx source,
       /* If this is a SYMBOL_REF that refers to a constant pool entry,
 	 and we have put it in the TOC, we just need to make a TOC-relative
 	 reference to it.  */
-      if (TARGET_TOC
-	  && SYMBOL_REF_P (operands[1])
-	  && use_toc_relative_ref (operands[1], mode))
+      if (use_toc_relative_ref (operands[1], mode))
 	operands[1] = create_TOC_reference (operands[1], operands[0]);
+
+      /* If this is a pc-relative reference, we don't have to do anything
+	 fancy.  */
+      else if (use_pc_relative_ref (operands[1]))
+	;
+
       else if (mode == Pmode
 	       && CONSTANT_P (operands[1])
 	       && GET_CODE (operands[1]) != HIGH
@@ -9919,14 +9947,18 @@ rs6000_emit_move (rtx dest, rtx source,
 
 	  operands[1] = force_const_mem (mode, operands[1]);
 
-	  if (TARGET_TOC
-	      && SYMBOL_REF_P (XEXP (operands[1], 0))
-	      && use_toc_relative_ref (XEXP (operands[1], 0), mode))
+	  rtx addr = XEXP (operands[1], 0);
+	  if (use_toc_relative_ref (addr, mode))
 	    {
-	      rtx tocref = create_TOC_reference (XEXP (operands[1], 0),
-						 operands[0]);
+	      rtx tocref = create_TOC_reference (addr, operands[0]);
 	      operands[1] = gen_const_mem (mode, tocref);
-	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
+	      set_mem_alias_set (operands[1], get_data_alias_set ());
+	    }
+
+	  else if (use_pc_relative_ref (addr))
+	    {
+	      operands[1] = gen_const_mem (mode, addr);
+	      set_mem_alias_set (operands[1], get_data_alias_set ());
 	    }
 	}
       break;
@@ -23732,14 +23764,16 @@ rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+/* Return the alias set for constants stored in either the TOC or via
+   pc-relative addressing.  */
+static GTY(()) alias_set_type data_alias_set = -1;
 
 alias_set_type
-get_TOC_alias_set (void)
+get_data_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (data_alias_set == -1)
+    data_alias_set = new_alias_set ();
+  return data_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 273255)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11606,15 +11606,23 @@ (define_insn_and_split "*cmp<mode>_inter
   operands[15] = force_const_mem (DFmode,
 				  const_double_from_real_value (dconst0,
 								DFmode));
-  if (TARGET_TOC)
+  if (TARGET_PCREL)
+    {
+      operands[14] = gen_const_mem (DFmode, XEXP (operands[14], 0));
+      operands[15] = gen_const_mem (DFmode, XEXP (operands[15], 0));
+      set_mem_alias_set (operands[14], get_data_alias_set ());
+      set_mem_alias_set (operands[15], get_data_alias_set ());
+    }
+
+  else if (TARGET_TOC)
     {
       rtx tocref;
       tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]);
       operands[14] = gen_const_mem (DFmode, tocref);
       tocref = create_TOC_reference (XEXP (operands[15], 0), operands[11]);
       operands[15] = gen_const_mem (DFmode, tocref);
-      set_mem_alias_set (operands[14], get_TOC_alias_set ());
-      set_mem_alias_set (operands[15], get_TOC_alias_set ());
+      set_mem_alias_set (operands[14], get_data_alias_set ());
+      set_mem_alias_set (operands[15], get_data_alias_set ());
     }
 })
 \f


-- 
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], PowerPC, Patch #6, Create pc-relative addressing insns

[Segher Boessenkool]

Hi Mike,

On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
>  static bool
>  use_toc_relative_ref (rtx sym, machine_mode mode)
>  {
> -  return ((constant_pool_expr_p (sym)
> -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> -					       get_pool_mode (sym)))
> -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> -	      && SYMBOL_REF_LOCAL_P (sym)
> -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> +    return false;

Why the SYMBOL_REF test?  The original didn't have any.  But its comment
says
  /* Return true iff the given SYMBOL_REF refers to a constant pool entry
     that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
     can be addressed relative to the toc pointer.  */
so perhaps you want an assert instead.

> +
> +  if (constant_pool_expr_p (sym)
> +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> +					  get_pool_mode (sym)))
> +    return true;
> +
> +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);

Please don't put disparate things on one line, it was fine before.

> +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
> +   have put in the pc-relative constant area, or for cmodel=medium, if the
> +   SYMBOL_REF can be addressed via pc-relative instructions.  */
> +
> +static bool
> +use_pc_relative_ref (rtx sym)
> +{
> +  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> +    return false;

Same here, assert please.  Or nothing, it will ICE not much later anyway.
But don't silently return if something violates the call contract.

> -static GTY(()) alias_set_type set = -1;
> +/* Return the alias set for constants stored in either the TOC or via
> +   pc-relative addressing.  */
> +static GTY(()) alias_set_type data_alias_set = -1;

Please just make a separate alias set for pcrel.  The new name isn't as
bad as just "set" :-), but "data"?  That's not great either.  Conflating
toc and pcrel isn't a good thing.

(Variables don't "return" anything btw).

>  
>  alias_set_type
> -get_TOC_alias_set (void)
> +get_data_alias_set (void)

This name is much worse than the old one.  Just make separate functions
for TOC and for pcrel?  Or is there any reason you want to share them?


Segher

Re: [PATCH], PowerPC, Patch #6, Create pc-relative addressing insns

[Michael Meissner]

On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> > @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
> >  static bool
> >  use_toc_relative_ref (rtx sym, machine_mode mode)
> >  {
> > -  return ((constant_pool_expr_p (sym)
> > -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > -					       get_pool_mode (sym)))
> > -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> > -	      && SYMBOL_REF_LOCAL_P (sym)
> > -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> > +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> > +    return false;
> 
> Why the SYMBOL_REF test?  The original didn't have any.  But its comment
> says
>   /* Return true iff the given SYMBOL_REF refers to a constant pool entry
>      that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
>      can be addressed relative to the toc pointer.  */
> so perhaps you want an assert instead.

The only two callers had a test for SYMBOL_REF_P.  By moving it into the
function, it made the call to the function one line instead of two.  But I can
go back to the previous method.

> > +
> > +  if (constant_pool_expr_p (sym)
> > +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > +					  get_pool_mode (sym)))
> > +    return true;
> > +
> > +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> > +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
> 
> Please don't put disparate things on one line, it was fine before.

I'm not sure what you mean, I was just trying to break up a long if statement.

> > +/* Return true iff the given SYMBOL_REF refers to a constant pool entry that we
> > +   have put in the pc-relative constant area, or for cmodel=medium, if the
> > +   SYMBOL_REF can be addressed via pc-relative instructions.  */
> > +
> > +static bool
> > +use_pc_relative_ref (rtx sym)
> > +{
> > +  if (!SYMBOL_REF_P (sym) || !TARGET_PCREL)
> > +    return false;
> 
> Same here, assert please.  Or nothing, it will ICE not much later anyway.
> But don't silently return if something violates the call contract.

Again, this was meant to simplify the call.

> > -static GTY(()) alias_set_type set = -1;
> > +/* Return the alias set for constants stored in either the TOC or via
> > +   pc-relative addressing.  */
> > +static GTY(()) alias_set_type data_alias_set = -1;
> 
> Please just make a separate alias set for pcrel.  The new name isn't as
> bad as just "set" :-), but "data"?  That's not great either.  Conflating
> toc and pcrel isn't a good thing.
> 
> (Variables don't "return" anything btw).
> 
> >  
> >  alias_set_type
> > -get_TOC_alias_set (void)
> > +get_data_alias_set (void)
> 
> This name is much worse than the old one.  Just make separate functions
> for TOC and for pcrel?  Or is there any reason you want to share them?

Well in theory, an object file that contains some functions wtih pc-relative
and some with TOC (due to #pragma target or attribute), using the same data set
would flag that they are related, but I imagine in practice the two uses don't
mix.  It was more of a hangover from trying to have one function to create both
addressing forms.

-- 
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], PowerPC, Patch #6, Create pc-relative addressing insns

[Segher Boessenkool]

On Wed, Jul 10, 2019 at 01:56:07PM -0400, Michael Meissner wrote:
> On Wed, Jul 10, 2019 at 12:11:49PM -0500, Segher Boessenkool wrote:
> > On Tue, Jul 09, 2019 at 06:30:00PM -0400, Michael Meissner wrote:
> > > @@ -8760,12 +8762,34 @@ rs6000_cannot_force_const_mem (machine_m
> > >  static bool
> > >  use_toc_relative_ref (rtx sym, machine_mode mode)
> > >  {
> > > -  return ((constant_pool_expr_p (sym)
> > > -	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > > -					       get_pool_mode (sym)))
> > > -	  || (TARGET_CMODEL == CMODEL_MEDIUM
> > > -	      && SYMBOL_REF_LOCAL_P (sym)
> > > -	      && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT));
> > > +  if (!SYMBOL_REF_P (sym) || TARGET_PCREL || !TARGET_TOC)
> > > +    return false;
> > 
> > Why the SYMBOL_REF test?  The original didn't have any.  But its comment
> > says
> >   /* Return true iff the given SYMBOL_REF refers to a constant pool entry
> >      that we have put in the TOC, or for cmodel=medium, if the SYMBOL_REF
> >      can be addressed relative to the toc pointer.  */
> > so perhaps you want an assert instead.
> 
> The only two callers had a test for SYMBOL_REF_P.  By moving it into the
> function, it made the call to the function one line instead of two.  But I can
> go back to the previous method.

It makes more sense with the param as a symbol always (the function
comment says it is, too, and the "sym" name suggests it is).  So yes,
please go back to that.  Reducing number of lines of code isn't a goal;
reducing complexity is, reducing astonishment.

> > > +
> > > +  if (constant_pool_expr_p (sym)
> > > +      && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (sym),
> > > +					  get_pool_mode (sym)))
> > > +    return true;
> > > +
> > > +  return (TARGET_CMODEL == CMODEL_MEDIUM && SYMBOL_REF_LOCAL_P (sym)
> > > +	  && GET_MODE_SIZE (mode) <= POWERPC64_TOC_POINTER_ALIGNMENT);
> > 
> > Please don't put disparate things on one line, it was fine before.
> 
> I'm not sure what you mean, I was just trying to break up a long if statement.

"TARGET_CMODEL == CMODEL_MEDIUM" and "SYMBOL_REF_LOCAL_P (sym)" are quite
different things.

> > > -static GTY(()) alias_set_type set = -1;
> > > +/* Return the alias set for constants stored in either the TOC or via
> > > +   pc-relative addressing.  */
> > > +static GTY(()) alias_set_type data_alias_set = -1;
> > 
> > Please just make a separate alias set for pcrel.  The new name isn't as
> > bad as just "set" :-), but "data"?  That's not great either.  Conflating
> > toc and pcrel isn't a good thing.
> > 
> > (Variables don't "return" anything btw).
> > 
> > >  alias_set_type
> > > -get_TOC_alias_set (void)
> > > +get_data_alias_set (void)
> > 
> > This name is much worse than the old one.  Just make separate functions
> > for TOC and for pcrel?  Or is there any reason you want to share them?
> 
> Well in theory, an object file that contains some functions wtih pc-relative
> and some with TOC (due to #pragma target or attribute), using the same data set
> would flag that they are related, but I imagine in practice the two uses don't
> mix.  It was more of a hangover from trying to have one function to create both
> addressing forms.

I think it would make things quite a bit simpler if you split them up.
Could you please try that?


Segher

[PATCH], PowerPC, Patch #6, revision 2, Create pc-relative addressing insns

[Michael Meissner]

Here is the revision of patch #6.  Changes from the previous version of the
patch:

1) I provided separate get_TOC_alias_set and get_pc_relative_alias_set
functions, and changed all of the places that had been changed to call
get_data_alias_set to revert to calling get_TOC_alias_set.

2) I added an assert in create_TOC_reference to guard against being called with
pc-relative addressing.

3) In the other places that check for TOC handling, I added tests to skip the
TOC handling if we are generating pc-relative code.

4) I added code in rs6000_emit_move to handle the local SYMBOL_REFs when
pc-relative moves are handled, as well as the constants that were created.

5) I simplified some of the code that set up prefixed addressing since we
already created a valid address when the constant was forced to memory.  I
realized that I didn't have to check whether the SYMBOL_REF was within the
constant pool with pc-relative addressing, I just had to check if it was a
local label and medium code model.

I have bootstrapped this on a little endian power8 system and there were no
regressions in the test suite.  Can I check this into the trunk?

2019-07-10  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-protos.h (get_pc_relative_alias_set): New
	declaration.
	* config/rs6000/rs6000.c (create_TOC_reference): Add gcc_assert.
	(toc_relative_expr_p): Add pc-relative check.
	(rs6000_legitimize_address): Add pc-relative check in TOC code.
	(rs6000_emit_move): Add support for loading up the addresses and
	constants with pc-relative addressing.
	(TOC_alias_set): Rename static variable from 'set'.
	(get_TOC_alias_set): Use TOC_alias_set.
	(pc_relative_alias_set): New static variable.
	(get_pc_relative_alias_set): New function.
	* config/rs6000/rs6000.md (cmp<mode>_internal2): Add support for
	pc-relative addressing.

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273361)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -190,6 +190,7 @@ extern void output_function_profiler (FI
 extern void output_profile_hook  (int);
 extern int rs6000_trampoline_size (void);
 extern alias_set_type get_TOC_alias_set (void);
+extern alias_set_type get_pc_relative_alias_set (void);
 extern void rs6000_emit_prologue (void);
 extern void rs6000_emit_load_toc_table (int);
 extern unsigned int rs6000_dbx_register_number (unsigned int, unsigned int);
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273363)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -7740,6 +7740,8 @@ create_TOC_reference (rtx symbol, rtx la
 {
   rtx tocrel, tocreg, hi;
 
+  gcc_assert (TARGET_TOC && !TARGET_PCREL);
+
   if (TARGET_DEBUG_ADDR)
     {
       if (SYMBOL_REF_P (symbol))
@@ -7785,7 +7787,7 @@ bool
 toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
 		     const_rtx *tocrel_offset_ret)
 {
-  if (!TARGET_TOC)
+  if (!TARGET_TOC || TARGET_PCREL)
     return false;
 
   if (TARGET_CMODEL != CMODEL_SMALL)
@@ -8137,7 +8139,7 @@ rs6000_legitimize_address (rtx x, rtx ol
 	emit_insn (gen_macho_high (reg, x));
       return gen_rtx_LO_SUM (Pmode, reg, x);
     }
-  else if (TARGET_TOC
+  else if (TARGET_TOC && !TARGET_PCREL
 	   && SYMBOL_REF_P (x)
 	   && constant_pool_expr_p (x)
 	   && ASM_OUTPUT_SPECIAL_POOL_ENTRY_P (get_pool_constant (x), Pmode))
@@ -9865,10 +9867,18 @@ rs6000_emit_move (rtx dest, rtx source,
       /* If this is a SYMBOL_REF that refers to a constant pool entry,
 	 and we have put it in the TOC, we just need to make a TOC-relative
 	 reference to it.  */
-      if (TARGET_TOC
+      if (TARGET_TOC && !TARGET_PCREL
 	  && SYMBOL_REF_P (operands[1])
 	  && use_toc_relative_ref (operands[1], mode))
 	operands[1] = create_TOC_reference (operands[1], operands[0]);
+
+      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,
+	 we don't have to do any special for it.  */
+      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
+	       && TARGET_CMODEL == CMODEL_MEDIUM
+	       && SYMBOL_REF_LOCAL_P (operands[1]))
+	;
+
       else if (mode == Pmode
 	       && CONSTANT_P (operands[1])
 	       && GET_CODE (operands[1]) != HIGH
@@ -9919,7 +9929,7 @@ rs6000_emit_move (rtx dest, rtx source,
 
 	  operands[1] = force_const_mem (mode, operands[1]);
 
-	  if (TARGET_TOC
+	  if (TARGET_TOC && !TARGET_PCREL
 	      && SYMBOL_REF_P (XEXP (operands[1], 0))
 	      && use_toc_relative_ref (XEXP (operands[1], 0), mode))
 	    {
@@ -9928,6 +9938,11 @@ rs6000_emit_move (rtx dest, rtx source,
 	      operands[1] = gen_const_mem (mode, tocref);
 	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
 	    }
+
+	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))
+		   && TARGET_CMODEL == CMODEL_MEDIUM
+		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))
+	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());
 	}
       break;
 
@@ -23732,14 +23747,26 @@ rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+/* Alias set for constants accessed via the TOC.  */
+static GTY(()) alias_set_type TOC_alias_set = -1;
 
 alias_set_type
 get_TOC_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (TOC_alias_set == -1)
+    TOC_alias_set = new_alias_set ();
+  return TOC_alias_set;
+}
+
+/* Alias set for constants accessed via the pc-relative addressing.  */
+static GTY(()) alias_set_type pc_relative_alias_set = -1;
+
+alias_set_type
+get_pc_relative_alias_set (void)
+{
+  if (pc_relative_alias_set == -1)
+    pc_relative_alias_set = new_alias_set ();
+  return pc_relative_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 273361)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -11606,7 +11606,7 @@ (define_insn_and_split "*cmp<mode>_inter
   operands[15] = force_const_mem (DFmode,
 				  const_double_from_real_value (dconst0,
 								DFmode));
-  if (TARGET_TOC)
+  if (TARGET_TOC && !TARGET_PCREL)
     {
       rtx tocref;
       tocref = create_TOC_reference (XEXP (operands[14], 0), operands[11]);
@@ -11616,6 +11616,12 @@ (define_insn_and_split "*cmp<mode>_inter
       set_mem_alias_set (operands[14], get_TOC_alias_set ());
       set_mem_alias_set (operands[15], get_TOC_alias_set ());
     }
+
+  else if (TARGET_PCREL)
+    {
+      set_mem_alias_set (operands[14], get_pc_relative_alias_set ());
+      set_mem_alias_set (operands[15], get_pc_relative_alias_set ());
+    }
 })
 \f
 ;; Now we have the scc insns.  We can do some combinations because of the

-- 
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], PowerPC, Patch #6, revision 2, Create pc-relative addressing insns

[Segher Boessenkool]

On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:
> +extern alias_set_type get_pc_relative_alias_set (void);

I'd just call it "pcrel", not "pc_relative", just like everywhere else?

> @@ -7785,7 +7787,7 @@ bool
>  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
>  		     const_rtx *tocrel_offset_ret)
>  {
> -  if (!TARGET_TOC)
> +  if (!TARGET_TOC || TARGET_PCREL)

Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or
maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't
make much sense at all to have both enabled, does it?

> +      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,
> +	 we don't have to do any special for it.  */
> +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
> +	       && TARGET_CMODEL == CMODEL_MEDIUM
> +	       && SYMBOL_REF_LOCAL_P (operands[1]))

      else if (TARGET_PCREL
	       && TARGET_CMODEL == CMODEL_MEDIUM
	       && SYMBOL_REF_P (operands[1])
	       && SYMBOL_REF_LOCAL_P (operands[1]))

> @@ -9928,6 +9938,11 @@ rs6000_emit_move (rtx dest, rtx source,
>  	      operands[1] = gen_const_mem (mode, tocref);
>  	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
>  	    }
> +
> +	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))
> +		   && TARGET_CMODEL == CMODEL_MEDIUM
> +		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))
> +	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());

Similar.

>  alias_set_type
>  get_TOC_alias_set (void)
>  {
> -  if (set == -1)
> -    set = new_alias_set ();
> -  return set;
> +  if (TOC_alias_set == -1)
> +    TOC_alias_set = new_alias_set ();
> +  return TOC_alias_set;
> +}

It would be nice if you could initialise the alias sets some other way.
Not new code, but you duplicate it ;-)

> +alias_set_type
> +get_pc_relative_alias_set (void)
> +{
> +  if (pc_relative_alias_set == -1)
> +    pc_relative_alias_set = new_alias_set ();
> +  return pc_relative_alias_set;
>  }

Rest looks fine.


Segher

Re: [PATCH], PowerPC, Patch #6, revision 2, Create pc-relative addressing insns

[Michael Meissner]

On Thu, Jul 11, 2019 at 02:52:03PM -0500, Segher Boessenkool wrote:
> On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:
> > +extern alias_set_type get_pc_relative_alias_set (void);
> 
> I'd just call it "pcrel", not "pc_relative", just like everywhere else?
> 
> > @@ -7785,7 +7787,7 @@ bool
> >  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
> >  		     const_rtx *tocrel_offset_ret)
> >  {
> > -  if (!TARGET_TOC)
> > +  if (!TARGET_TOC || TARGET_PCREL)
> 
> Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or
> maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't
> make much sense at all to have both enabled, does it?

Yeah, in theory TARGET_TOC should not be set if TARGET_PCREL is set, but when I
last tried it, there were some failures.  I can make a macro for the two together.

> > +      /* If this is a SYMBOL_REF that we refer to via pc-relative addressing,
> > +	 we don't have to do any special for it.  */
> > +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
> > +	       && TARGET_CMODEL == CMODEL_MEDIUM
> > +	       && SYMBOL_REF_LOCAL_P (operands[1]))
> 
>       else if (TARGET_PCREL
> 	       && TARGET_CMODEL == CMODEL_MEDIUM
> 	       && SYMBOL_REF_P (operands[1])
> 	       && SYMBOL_REF_LOCAL_P (operands[1]))
> 
> > @@ -9928,6 +9938,11 @@ rs6000_emit_move (rtx dest, rtx source,
> >  	      operands[1] = gen_const_mem (mode, tocref);
> >  	      set_mem_alias_set (operands[1], get_TOC_alias_set ());
> >  	    }
> > +
> > +	  else if (TARGET_PCREL && SYMBOL_REF_P (XEXP (operands[1], 0))
> > +		   && TARGET_CMODEL == CMODEL_MEDIUM
> > +		   && SYMBOL_REF_LOCAL_P (XEXP (operands[1], 0)))
> > +	    set_mem_alias_set (operands[1], get_pc_relative_alias_set ());
> 
> Similar.

I had it in a function that did the checking, but after eliminating the
constant pool check that TOC needs, it was just doing the medium code model and
symbol_ref local checks, so I eliminated the function.

> >  alias_set_type
> >  get_TOC_alias_set (void)
> >  {
> > -  if (set == -1)
> > -    set = new_alias_set ();
> > -  return set;
> > +  if (TOC_alias_set == -1)
> > +    TOC_alias_set = new_alias_set ();
> > +  return TOC_alias_set;
> > +}
> 
> It would be nice if you could initialise the alias sets some other way.
> Not new code, but you duplicate it ;-)

For the pc-relative case, I'm not sure we need the alias set.  In the TOC case,
we do a force_const_mem which returns a SYMBOL_REF pointing to the data in the
constant pool.  Then we call create_TOC_reference which rewrites the memory,
and then it calls gen_const_mem which notes that memory in unchanging.

But for pc-relative, we just use output of force_const_mem.  Now
force_const_mem does not seem to set the alias set (but it will have the
constant pool bit set).

> > +alias_set_type
> > +get_pc_relative_alias_set (void)
> > +{
> > +  if (pc_relative_alias_set == -1)
> > +    pc_relative_alias_set = new_alias_set ();
> > +  return pc_relative_alias_set;
> >  }
> 
> Rest looks fine.
> 
> 
> 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], PowerPC, Patch #6, revision 2, Create pc-relative addressing insns

[Segher Boessenkool]

On Thu, Jul 11, 2019 at 04:09:44PM -0400, Michael Meissner wrote:
> On Thu, Jul 11, 2019 at 02:52:03PM -0500, Segher Boessenkool wrote:
> > On Wed, Jul 10, 2019 at 05:17:48PM -0400, Michael Meissner wrote:
> > > +extern alias_set_type get_pc_relative_alias_set (void);
> > 
> > I'd just call it "pcrel", not "pc_relative", just like everywhere else?
> > 
> > > @@ -7785,7 +7787,7 @@ bool
> > >  toc_relative_expr_p (const_rtx op, bool strict, const_rtx *tocrel_base_ret,
> > >  		     const_rtx *tocrel_offset_ret)
> > >  {
> > > -  if (!TARGET_TOC)
> > > +  if (!TARGET_TOC || TARGET_PCREL)
> > 
> > Maybe (TARGET_TOC && !TARGET_PCREL) should get a name of its own?  Or
> > maybe TARGET_TOC should not be enabled when TARGET_PCREL is?  It doesn't
> > make much sense at all to have both enabled, does it?
> 
> Yeah, in theory TARGET_TOC should not be set if TARGET_PCREL is set, but when I
> last tried it, there were some failures.  I can make a macro for the two together.

TARGET_TOC is set in various header files (linux64.h etc.)  While
TARGET_PCREL is defined in rs6000.opt .  Maybe rename the original
TARGET_TOC definitions, and make a generic TARGET_TOC defined as
(TARGET_CAN_HAVE_TOC && !TARGET_PCREL) ?  Something like that?

> > > +      else if (TARGET_PCREL && SYMBOL_REF_P (operands[1])
> > > +	       && TARGET_CMODEL == CMODEL_MEDIUM
> > > +	       && SYMBOL_REF_LOCAL_P (operands[1]))
> > 
> >       else if (TARGET_PCREL
> > 	       && TARGET_CMODEL == CMODEL_MEDIUM
> > 	       && SYMBOL_REF_P (operands[1])
> > 	       && SYMBOL_REF_LOCAL_P (operands[1]))

> I had it in a function that did the checking, but after eliminating the
> constant pool check that TOC needs, it was just doing the medium code model and
> symbol_ref local checks, so I eliminated the function.

My point is that you should put like things together, and you should not
put unrelated conditions on one line.

You could put the SYMBOL_REF_P and SYMBOL_REF_LOCAL_P on one line, I
wouldn't mind (except that doesn't fit on one line then ;-) ).

> > >  alias_set_type
> > >  get_TOC_alias_set (void)
> > >  {
> > > -  if (set == -1)
> > > -    set = new_alias_set ();
> > > -  return set;
> > > +  if (TOC_alias_set == -1)
> > > +    TOC_alias_set = new_alias_set ();
> > > +  return TOC_alias_set;
> > > +}
> > 
> > It would be nice if you could initialise the alias sets some other way.
> > Not new code, but you duplicate it ;-)
> 
> For the pc-relative case, I'm not sure we need the alias set.  In the TOC case,
> we do a force_const_mem which returns a SYMBOL_REF pointing to the data in the
> constant pool.  Then we call create_TOC_reference which rewrites the memory,
> and then it calls gen_const_mem which notes that memory in unchanging.
> 
> But for pc-relative, we just use output of force_const_mem.  Now
> force_const_mem does not seem to set the alias set (but it will have the
> constant pool bit set).

I mean just this:
  if (TOC_alias_set == -1)
    TOC_alias_set = new_alias_set ();
in the getter function.  It could be initialised elsewhere, there are
enough initialisation functions, it will fit *somewhere*, and then the
getter function doesn't need to do the initialisation anymore.  Which
then suddenly makes it very inlinable, etc.


Segher

[PATCH], PowerPC, Patch #7, Split up SIGNED_34BIT and SIGNED_16BIT macros

[Michael Meissner]

This patch splits up the macros SIGNED_16BIT_OFFSET_P and SIGNED_34BIT_OFFSET_P
into two separate macros as you asked for previously in private mail.  The main
macros:

	SIGNED_16BIT_OFFSET_P
	SIGNED_34BIT_OFFSET_P

only take one argument, and that is the offset that is being tested.  The new
macros:

	SIGNED_16BIT_OFFSET_EXTRA_P
	SIGNED_34BIT_OFFSET_EXTRA_P

Retain the two arguments that the current macros have.  It is useful when the
functions that are validating addresses that might be split (such as the two
doubles in __ibm128) to verify that all addresses in the range of offset to
offset + extra are valid 16 or 34-bit offsets.  I have changed the existing
uses of these macros.

I have bootstrapped the compiler on a little endian power8 machine and there
were no regressions in the test suite.  Can I check this change into the trunk?

2019-07-09  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (cint34_operand): Update
	SIGNED_34BIT_OFFSET_P call.
	(pcrel_address): Update SIGNED_34BIT_OFFSET_P call.
	(pcrel_external_address): Update SIGNED_34BIT_OFFSET_P call.
	* config/rs6000/rs6000.c (rs6000_prefixed_address): Update
	SIGNED_16BIT_OFFSET_P and SIGNED_34BIT_OFFSET_P calls.
	* config/rs6000/rs6000.h (SIGNED_16BIT_OFFSET_P): Remove EXTRA
	argument.
	(SIGNED_34BIT_OFFSET_P): Remove EXTRA argument.
	(SIGNED_16BIT_OFFSET_EXTRA_P): New macro, like
	SIGNED_16BIT_OFFSET_P with an EXTRA argument.
	(SIGNED_34BIT_OFFSET_EXTRA_P): New macro, like
	SIGNED_34BIT_OFFSET_P with an EXTRA argument.

Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 273255)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -309,7 +309,7 @@ (define_predicate "cint34_operand"
   if (!TARGET_PREFIXED_ADDR)
     return 0;
 
-  return SIGNED_34BIT_OFFSET_P (INTVAL (op), 0);
+  return SIGNED_34BIT_OFFSET_P (INTVAL (op));
 })
 
 ;; Return 1 if op is a register that is not special.
@@ -1638,7 +1638,7 @@ (define_predicate "pcrel_address"
       rtx op0 = XEXP (op, 0);
       rtx op1 = XEXP (op, 1);
 
-      if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1), 0))
+      if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1)))
 	return false;
 
       op = op0;
@@ -1673,7 +1673,7 @@ (define_predicate "pcrel_external_addres
       rtx op0 = XEXP (op, 0);
       rtx op1 = XEXP (op, 1);
 
-      if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1), 0))
+      if (!CONST_INT_P (op1) || !SIGNED_34BIT_OFFSET_P (INTVAL (op1)))
 	return false;
 
       op = op0;
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273313)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -21551,11 +21551,11 @@ rs6000_prefixed_address (rtx addr, machi
 	return false;
 
       HOST_WIDE_INT value = INTVAL (op1);
-      if (!SIGNED_34BIT_OFFSET_P (value, 0))
+      if (!SIGNED_34BIT_OFFSET_P (value))
 	return false;
 
       /* Offset larger than 16-bits?  */
-      if (!SIGNED_16BIT_OFFSET_P (value, 0))
+      if (!SIGNED_16BIT_OFFSET_P (value))
 	return true;
 
       /* DQ instruction (bottom 4 bits must be 0) for vectors.  */
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 273255)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -2526,16 +2526,27 @@ typedef struct GTY(()) machine_function
 #pragma GCC poison TARGET_FLOAT128 OPTION_MASK_FLOAT128 MASK_FLOAT128
 #endif
 
-/* Whether a given VALUE is a valid 16- or 34-bit signed offset.  EXTRA is the
-   amount that we can't touch at the high end of the range (typically if the
-   address is split into smaller addresses, the extra covers the addresses
-   which might be generated when the insn is split).  */
-#define SIGNED_16BIT_OFFSET_P(VALUE, EXTRA)				\
-  IN_RANGE (VALUE,							\
+/* Whether a given VALUE is a valid 16 or 34-bit signed offset.  */
+#define SIGNED_16BIT_OFFSET_P(VALUE)					\
+  IN_RANGE ((VALUE),							\
+	    -(HOST_WIDE_INT_1 << 15),					\
+	    (HOST_WIDE_INT_1 << 15) - 1)
+
+#define SIGNED_34BIT_OFFSET_P(VALUE)					\
+  IN_RANGE ((VALUE),							\
+	    -(HOST_WIDE_INT_1 << 33),					\
+	    (HOST_WIDE_INT_1 << 33) - 1)
+
+/* Like SIGNED_16BIT_OFFSET_P and SIGNED_34BIT_OFFSET_P, but with an extra
+   argument that gives a length to validate a range of addresses, to allow for
+   splitting insns into several insns, each of which has an offsettable
+   address.  */
+#define SIGNED_16BIT_OFFSET_EXTRA_P(VALUE, EXTRA)			\
+  IN_RANGE ((VALUE),							\
 	    -(HOST_WIDE_INT_1 << 15),					\
 	    (HOST_WIDE_INT_1 << 15) - 1 - (EXTRA))
 
-#define SIGNED_34BIT_OFFSET_P(VALUE, EXTRA)				\
-  IN_RANGE (VALUE,							\
+#define SIGNED_34BIT_OFFSET_EXTRA_P(VALUE, EXTRA)			\
+  IN_RANGE ((VALUE),							\
 	    -(HOST_WIDE_INT_1 << 33),					\
 	    (HOST_WIDE_INT_1 << 33) - 1 - (EXTRA))


-- 
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], PowerPC, Patch #7, Split up SIGNED_34BIT and SIGNED_16BIT macros

[Segher Boessenkool]

On Tue, Jul 09, 2019 at 07:46:26PM -0400, Michael Meissner wrote:
> This patch splits up the macros SIGNED_16BIT_OFFSET_P and SIGNED_34BIT_OFFSET_P
> into two separate macros as you asked for previously in private mail.  The main
> macros:
> 
> 	SIGNED_16BIT_OFFSET_P
> 	SIGNED_34BIT_OFFSET_P
> 
> only take one argument, and that is the offset that is being tested.  The new
> macros:
> 
> 	SIGNED_16BIT_OFFSET_EXTRA_P
> 	SIGNED_34BIT_OFFSET_EXTRA_P
> 
> Retain the two arguments that the current macros have.  It is useful when the
> functions that are validating addresses that might be split (such as the two
> doubles in __ibm128) to verify that all addresses in the range of offset to
> offset + extra are valid 16 or 34-bit offsets.  I have changed the existing
> uses of these macros.

This is okay for trunk.  Thanks Mike.


Segher


> 	* config/rs6000/predicates.md (cint34_operand): Update
> 	SIGNED_34BIT_OFFSET_P call.
> 	(pcrel_address): Update SIGNED_34BIT_OFFSET_P call.
> 	(pcrel_external_address): Update SIGNED_34BIT_OFFSET_P call.
> 	* config/rs6000/rs6000.c (rs6000_prefixed_address): Update
> 	SIGNED_16BIT_OFFSET_P and SIGNED_34BIT_OFFSET_P calls.
> 	* config/rs6000/rs6000.h (SIGNED_16BIT_OFFSET_P): Remove EXTRA
> 	argument.
> 	(SIGNED_34BIT_OFFSET_P): Remove EXTRA argument.
> 	(SIGNED_16BIT_OFFSET_EXTRA_P): New macro, like
> 	SIGNED_16BIT_OFFSET_P with an EXTRA argument.
> 	(SIGNED_34BIT_OFFSET_EXTRA_P): New macro, like
> 	SIGNED_34BIT_OFFSET_P with an EXTRA argument.

[PATCH], PowerPC, Patch #8, rename rs6000_prefixed_address

[Michael Meissner]

In a future patch, I plan to introduce a new function that says whether an
address is prefixed based on the address format (D-form, DS-form, DQ-form) used
by the instruction.  I plan on naming the new function:

	rs6000_prefixed_address_format

This means the existing function that takes a mode argument will be renamed to:

	rs6000_prefixed_address_mode

Rs6000_prefixed_address_mode will have a table mapping the mode into the
expected address format, and then call rs6000_prefixed_address_format.  This is
due to the number of irregularities in the PowerPC architecture:

	LWA is DS-form, but LWZ is D-form
	LFD is D-form, LXSD is DS-form

In the tests that will occur after register allocation, we can check the
register type, and more accurately determine if a prefixed instruction must be
used if the offset is odd (for DS-form) or the offset is not aligned on a
16-byte boundary (for DQ-form).

This patch prepares the way by renaming the current function, and changing its
one use.  I have bootstrapped a compiler on a little endian power8 and there
were no regressions in the test suite.  Can I check this patch into the trunk?

2019-07-10  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (prefixed_mem_operand): Call
	rs6000_prefixed_address_mode instead of rs6000_prefixed_address.
	* config/rs6000/rs6000-protos.h (rs6000_prefixed_address_mode):
	Rename fucntion from rs6000_prefixed_address.
	* config/rs6000/rs6000.c (rs6000_prefixed_address_mode): Rename
	fucntion from rs6000_prefixed_address.

Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 273368)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -1686,7 +1686,7 @@ (define_predicate "pcrel_external_addres
 (define_predicate "prefixed_mem_operand"
   (match_code "mem")
 {
-  return rs6000_prefixed_address (XEXP (op, 0), GET_MODE (op));
+  return rs6000_prefixed_address_mode (XEXP (op, 0), GET_MODE (op));
 })
 
 ;; Return 1 if op is a memory operand to an external variable when we
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273367)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -154,7 +154,7 @@ extern align_flags rs6000_loop_align (rt
 extern void rs6000_split_logical (rtx [], enum rtx_code, bool, bool, bool);
 extern bool rs6000_pcrel_p (struct function *);
 extern bool rs6000_fndecl_pcrel_p (const_tree);
-extern bool rs6000_prefixed_address (rtx, machine_mode);
+extern bool rs6000_prefixed_address_mode (rtx, machine_mode);
 #endif /* RTX_CODE */
 
 #ifdef TREE_CODE
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273368)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -21514,7 +21514,7 @@ mode_supports_prefixed_address_p (machin
    mode MODE.  */
 
 bool
-rs6000_prefixed_address (rtx addr, machine_mode mode)
+rs6000_prefixed_address_mode (rtx addr, machine_mode mode)
 {
   if (!TARGET_PREFIXED_ADDR || !mode_supports_prefixed_address_p (mode))
     return false;

-- 
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], PowerPC, Patch #8, rename rs6000_prefixed_address

[Segher Boessenkool]

[ Please do not send patches as replies. ]

On Thu, Jul 11, 2019 at 08:11:52AM -0400, Michael Meissner wrote:
> In a future patch, I plan to introduce a new function that says whether an
> address is prefixed based on the address format (D-form, DS-form, DQ-form) used
> by the instruction.  I plan on naming the new function:
> 
> 	rs6000_prefixed_address_format
> 
> This means the existing function that takes a mode argument will be renamed to:
> 
> 	rs6000_prefixed_address_mode

D/DQ/DS are the "instruction form", not "address format".  Let's not
invent new similar names :-/

> Rs6000_prefixed_address_mode will have a table mapping the mode into the
> expected address format, and then call rs6000_prefixed_address_format.  This is
> due to the number of irregularities in the PowerPC architecture:
> 
> 	LWA is DS-form, but LWZ is D-form
> 	LFD is D-form, LXSD is DS-form

Great isn't it?

> In the tests that will occur after register allocation, we can check the
> register type, and more accurately determine if a prefixed instruction must be
> used if the offset is odd (for DS-form) or the offset is not aligned on a
> 16-byte boundary (for DQ-form).

Is not 0 mod 4, for DS, even.

> --- gcc/config/rs6000/predicates.md	(revision 273368)
> +++ gcc/config/rs6000/predicates.md	(working copy)
> @@ -1686,7 +1686,7 @@ (define_predicate "pcrel_external_addres
>  (define_predicate "prefixed_mem_operand"
>    (match_code "mem")
>  {
> -  return rs6000_prefixed_address (XEXP (op, 0), GET_MODE (op));
> +  return rs6000_prefixed_address_mode (XEXP (op, 0), GET_MODE (op));
>  })

That sounds like it returns a mode.  rs6000_prefixed_address_mode_p or
rs6000_is_prefixed_address_mode, maybe?  Well you use _p alredy, so
please do that here as well.

Okay for trunk with that change.  Thanks!


Segher

[PATCH], PowerPC, Patch #9, Refine calculation of whether an address offset is d-form, ds-form, or dq-form

[Michael Meissner]

As I mentioned in patch #8, I needed to refine the calculation of what type of
address offset it used on the PowerPC for particular loads and stores.

The motivation is that the new prefixed instructions that future processors
may support have improved instruction encodings, and the prefixed form of the
instruction do not need the bottom 2 bits clear for traditional DS-form
instructions or the bottom 4 bits clear for traditional DQ-form instructions.

This means if you have a load where the offset it not aligned:

	LD rx,3(base)

currently the compiler will have to generate code like:

	LI tmp,3
	LDX rx,tmp,base

With prefixed instructions, the compiler can instead generate:

	PLD rx,3(base)

The original implementation just using the mode does not work in a lot of
cases, because you don't know the context of the use.

For example, a LWZ instruction uses a d-form load (all 16 bits are available),
while a LWA instruction uses a ds-form load (bottom 2 bits must be 0).

Another example is loading up a scalar floating value.  If we are loading to a
traditional FPR, the LFD instruction is a d-form instruction, while if we are
loading to a traditional Altivec register, the LXSD instruction is a ds-form
instruction.

I have built the compiler with these patches installed on a little endian
power8 system.  There were no regressions in the test suite.  Can I install
this patch into the FSF trunk?

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

	* config/rs6000/rs6000-protos.h (enum rs6000_offset_format): New
	enumeration to describe the addressing offset format.
	(reg_to_offset_format): New declaration.
	(rs6000_prefixed_address_format): New declaration.
	* config/rs6000/rs6000.c (struct rs6000_reg_addr): Add default
	address offset format field.
	(mode_supports_prefixed_address_p): Move function higher.
	(initialize_offset_formats): New function to determine the default
	address format for each mode.
	(rs6000_init_hard_regno_mode_ok): Initialize the address formats
	for each mode.
	(quad_address_p): Add support for prefixed instructions.
	(mem_operand_gpr): Add support for prefixed instructions.
	(mem_operand_ds_form): Add support for prefixed instructions.
	(rs6000_prefixed_address_format): New function to validate an
	address given an address format.
	(rs6000_prefixed_address_mode): Rewrite to used saved default
	address mode format.
	(reg_to_offset_format): New function to determine what address
	format to use for a particular register.

Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 273371)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -154,6 +154,18 @@ extern align_flags rs6000_loop_align (rt
 extern void rs6000_split_logical (rtx [], enum rtx_code, bool, bool, bool);
 extern bool rs6000_pcrel_p (struct function *);
 extern bool rs6000_fndecl_pcrel_p (const_tree);
+
+/* Enumeration to describe the offsettable address formats of PowerPC memory
+   instructions.  */
+enum rs6000_offset_format {
+  OFFSET_FORMAT_NONE,		/* No offset format is available.  */
+  OFFSET_FORMAT_D,		/* All 16-bits are available.  */
+  OFFSET_FORMAT_DS,		/* Bottom 2 bits must be 0.  */
+  OFFSET_FORMAT_DQ		/* Bottom 4 bits must be 0.  */
+};
+
+extern enum rs6000_offset_format reg_to_offset_format (rtx, machine_mode, bool);
+extern bool rs6000_prefixed_address_format (rtx, enum rs6000_offset_format);
 extern bool rs6000_prefixed_address_mode (rtx, machine_mode);
 #endif /* RTX_CODE */
 
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273371)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -459,6 +459,7 @@ struct rs6000_reg_addr {
   enum insn_code reload_fpr_gpr;	/* INSN to move from FPR to GPR.  */
   enum insn_code reload_gpr_vsx;	/* INSN to move from GPR to VSX.  */
   enum insn_code reload_vsx_gpr;	/* INSN to move from VSX to GPR.  */
+  enum rs6000_offset_format offset_format;	/* Format of offset insn.  */
   addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
   bool scalar_in_vmx_p;			/* Scalar value can go in VMX.  */
 };
@@ -498,6 +499,17 @@ mode_supports_dq_form (machine_mode mode
 	  != 0);
 }
 
+/* Helper function to return whether a MODE can do prefixed loads/stores.
+   VOIDmode is used when we are loading the pc-relative address into a base
+   register, but we are not using it as part of a memory operation.  As modes
+   add support for prefixed memory, they will be added here.  */
+
+static bool
+mode_supports_prefixed_address_p (machine_mode mode)
+{
+  return mode == VOIDmode;
+}
+
 /* Given that there exists at least one variable that is set (produced)
    by OUT_INSN and read (consumed) by IN_INSN, return true iff
    IN_INSN represents one or more memory store operations and none of
@@ -2702,6 +2714,92 @@ rs6000_debug_reg_global (void)
 }
 
 \f
+/* Initiailize the table of offset address formats.  Unfortunately we do not
+   know the context of the use, so we have to return the address form to model
+   the expected usage.  */
+static void
+initialize_offset_formats (void)
+{
+  /* Set offset format for some types based on the switches.  */
+  enum rs6000_offset_format format_gpr_64bit
+    = (TARGET_POWERPC64) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
+  enum rs6000_offset_format format_gpr_128bit
+    = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;
+  enum rs6000_offset_format format_fpr_64bit
+    = (TARGET_SOFT_FLOAT) ? OFFSET_FORMAT_D : format_gpr_64bit;
+  enum rs6000_offset_format format_vector
+    = (TARGET_P9_VECTOR) ? OFFSET_FORMAT_DQ : OFFSET_FORMAT_NONE;
+  enum rs6000_offset_format format_tf
+    = (TARGET_IEEEQUAD) ? format_vector : format_fpr_64bit;
+
+  for (ssize_t m = 0; m < NUM_MACHINE_MODES; m++)
+    reg_addr[m].offset_format = OFFSET_FORMAT_NONE;
+
+  /* Small integers.  */
+  reg_addr[E_QImode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_HImode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CQImode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CHImode].offset_format = OFFSET_FORMAT_D;
+
+  /* Note, LWA needs ds-form, not d-form, but we set up SImode for the normal
+     load/store.  The Y constraint will prevent the a LWA with odd offset.  */
+  reg_addr[E_SImode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CSImode].offset_format = OFFSET_FORMAT_D;
+
+  /* While DImode can be loaded into a FPR register with LFD (d-form), the
+     normal use is to use a GPR (ds-form in 64-bit).  */
+  reg_addr[E_DImode].offset_format = format_gpr_64bit;
+  reg_addr[E_CDImode].offset_format = format_gpr_64bit;
+
+  /* Condition registers (uses LWZ/STW).  */
+  reg_addr[E_CCmode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CCUNSmode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CCFPmode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_CCEQmode].offset_format = OFFSET_FORMAT_D;
+
+  /* 128-bit integers.  */
+  if (TARGET_POWERPC64)
+    {
+      reg_addr[E_TImode].offset_format = format_vector;
+      reg_addr[E_PTImode].offset_format = format_gpr_128bit;
+      reg_addr[E_CTImode].offset_format = format_vector;
+      reg_addr[E_CPTImode].offset_format = format_gpr_128bit;
+    }
+
+  /* Scalar floating point.  While SFmode and DFmode need a ds-form load to be
+     loaded into an Altivec register, use d-form for the FPR registers.  The wY
+     constraint will prevent odd Altivec load/stores.  For SDmode, there isn't
+     an offsettable mode that can be used to load it into a FPR register.  */
+  reg_addr[E_SFmode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_SCmode].offset_format = OFFSET_FORMAT_D;
+  reg_addr[E_DFmode].offset_format = format_fpr_64bit;
+  reg_addr[E_DCmode].offset_format = format_fpr_64bit;
+  reg_addr[E_TFmode].offset_format = format_tf;
+  reg_addr[E_TCmode].offset_format = format_tf;
+  reg_addr[E_IFmode].offset_format = format_fpr_64bit;
+  reg_addr[E_ICmode].offset_format = format_fpr_64bit;
+  reg_addr[E_KFmode].offset_format = format_vector;
+  reg_addr[E_KCmode].offset_format = format_vector;
+  reg_addr[E_DDmode].offset_format = format_fpr_64bit;
+  reg_addr[E_TDmode].offset_format = format_fpr_64bit;
+
+  /* Vectors.  */
+  reg_addr[E_V16QImode].offset_format = format_vector;
+  reg_addr[E_V8HImode].offset_format = format_vector;
+  reg_addr[E_V4SImode].offset_format = format_vector;
+  reg_addr[E_V2DImode].offset_format = format_vector;
+  reg_addr[E_V1TImode].offset_format = format_vector;
+  reg_addr[E_V4SFmode].offset_format = format_vector;
+  reg_addr[E_V2DFmode].offset_format = format_vector;
+
+  /* VOIDmode and BLKmode get the most pessimistic format.  */
+  reg_addr[E_VOIDmode].offset_format = OFFSET_FORMAT_DQ;
+  reg_addr[E_BLKmode].offset_format = OFFSET_FORMAT_DQ;
+
+  return;
+}
+
+\f
 /* Update the addr mask bits in reg_addr to help secondary reload and go if
    legitimate address support to figure out the appropriate addressing to
    use.  */
@@ -3238,6 +3336,9 @@ rs6000_init_hard_regno_mode_ok (bool glo
 	}
     }
 
+  /* Initial the mapping of mode to offset formats.  */
+  initialize_offset_formats ();
+
   /* Precalculate HARD_REGNO_NREGS.  */
   for (r = 0; HARD_REGISTER_NUM_P (r); ++r)
     for (m = 0; m < NUM_MACHINE_MODES; ++m)
@@ -7407,6 +7508,14 @@ quad_address_p (rtx addr, machine_mode m
   if (VECTOR_MODE_P (mode) && !mode_supports_dq_form (mode))
     return false;
 
+  /* Is this a valid prefixed address?  If the bottom four bits of the offset
+     are non-zero, we could use a prefixed instruction (which does not have the
+     DQ-form constraint that the traditional instruction had) instead of
+     forcing the unaligned offset to a GPR.  */
+  if (mode_supports_prefixed_address_p (mode)
+      && rs6000_prefixed_address_format (addr, OFFSET_FORMAT_DQ))
+    return true;
+
   if (GET_CODE (addr) != PLUS)
     return false;
 
@@ -7508,6 +7617,14 @@ mem_operand_gpr (rtx op, machine_mode mo
       && legitimate_indirect_address_p (XEXP (addr, 0), false))
     return true;
 
+  /* Allow prefixed instructions if supported.  If the bottom two bits of the
+     offset are non-zero, we could use a prefixed instruction (which does not
+     have the DS-form constraint that the traditional instruction had) instead
+     of forcing the unaligned offset to a GPR.  */
+  if (mode_supports_prefixed_address_p (mode)
+      && rs6000_prefixed_address_format (XEXP (op, 0), OFFSET_FORMAT_DS))
+    return true;
+
   /* Don't allow non-offsettable addresses.  See PRs 83969 and 84279.  */
   if (!rs6000_offsettable_memref_p (op, mode, false))
     return false;
@@ -7542,6 +7659,14 @@ mem_operand_ds_form (rtx op, machine_mod
   int extra;
   rtx addr = XEXP (op, 0);
 
+  /* Allow prefixed instructions if supported.  If the bottom two bits of the
+     offset are non-zero, we could use a prefixed instruction (which does not
+     have the DS-form constraint that the traditional instruction had) instead
+     of forcing the unaligned offset to a GPR.  */
+  if (mode_supports_prefixed_address_p (mode)
+      && rs6000_prefixed_address_format (XEXP (op, 0), OFFSET_FORMAT_DS))
+    return true;
+
   if (!offsettable_address_p (false, mode, addr))
     return false;
 
@@ -21499,24 +21624,14 @@ rs6000_pltseq_template (rtx *operands, i
 }
 #endif
 
-/* Helper function to return whether a MODE can do prefixed loads/stores.
-   VOIDmode is used when we are loading the pc-relative address into a base
-   register, but we are not using it as part of a memory operation.  As modes
-   add support for prefixed memory, they will be added here.  */
-
-static bool
-mode_supports_prefixed_address_p (machine_mode mode)
-{
-  return mode == VOIDmode;
-}
-
-/* Function to return true if ADDR is a valid prefixed memory address that uses
-   mode MODE.  */
+\f
+/* Return true if ADDR is a valid prefixed memory address and the traditional
+   form of the instruction uses the address format FORM.  */
 
 bool
-rs6000_prefixed_address_mode (rtx addr, machine_mode mode)
+rs6000_prefixed_address_format (rtx addr, enum rs6000_offset_format format)
 {
-  if (!TARGET_PREFIXED_ADDR || !mode_supports_prefixed_address_p (mode))
+  if (!TARGET_PREFIXED_ADDR)
     return false;
 
   /* Check for PC-relative addresses.  */
@@ -21541,28 +21656,110 @@ rs6000_prefixed_address_mode (rtx addr,
       if (!SIGNED_16BIT_OFFSET_P (value))
 	return true;
 
-      /* DQ instruction (bottom 4 bits must be 0) for vectors.  */
-      HOST_WIDE_INT mask;
-      if (GET_MODE_SIZE (mode) >= 16)
-	mask = 15;
-
-      /* DS instruction (bottom 2 bits must be 0).  For 32-bit integers, we
-	 need to use DS instructions if we are sign-extending the value with
-	 LWA.  For 32-bit floating point, we need DS instructions to load and
-	 store values to the traditional Altivec registers.  */
-      else if (GET_MODE_SIZE (mode) >= 4)
-	mask = 3;
+      switch (format)
+	{
+	default:
+	  gcc_unreachable ();
+
+	case OFFSET_FORMAT_D:		/* All 16-bits are available.  */
+	  return false;
 
-      /* QImode/HImode has no restrictions.  */
-      else
-	return true;
+	case OFFSET_FORMAT_DS:		/* Bottom 2 bits must be 0.  */
+	  return (value & 3) != 0;
 
-      /* Return true if we must use a prefixed instruction.  */
-      return (value & mask) != 0;
+	case OFFSET_FORMAT_NONE:
+	case OFFSET_FORMAT_DQ:		/* Bottom 4 bits must be 0.  */
+	  return (value & 15) != 0;
+	}
     }
 
   return false;
 }
+
+/* Return true if ADDR is a valid prefixed memory address that uses mode
+   MODE.  */
+
+bool
+rs6000_prefixed_address_mode (rtx addr, machine_mode mode)
+{
+  if (!TARGET_PREFIXED_ADDR || !mode_supports_prefixed_address_p (mode))
+    return false;
+
+  return rs6000_prefixed_address_format (addr, reg_addr[mode].offset_format);
+}
+
+\f
+/* For a given hard register and whether it is being sign extended, return the
+   address format (d-form, ds-form, dq-form).  */
+
+enum rs6000_offset_format
+reg_to_offset_format (rtx reg, machine_mode mode, bool sign_extend_p)
+{
+  unsigned int msize = GET_MODE_SIZE (mode);
+  bool altivec_reg = false;
+  enum rs6000_reg_type rtype = register_to_reg_type (reg, &altivec_reg);
+  enum rs6000_offset_format format = OFFSET_FORMAT_NONE;
+
+  /* LWA (SImode -> DImode) is ds-form, but normal LWZ is d-form.  */
+  if (mode == SImode && sign_extend_p)
+    return OFFSET_FORMAT_DS;
+
+  switch (rtype)
+    {
+      /* If the register hasn't been allocated yet, use the default format.  */
+    case NO_REG_TYPE:
+    case PSEUDO_REG_TYPE:
+      format = reg_addr[mode].offset_format;
+      break;
+
+      /* LHZ, LWZ, etc. are d-form, LD is ds-format, LQ is dq-form.  */
+    case GPR_REG_TYPE:
+      if (msize < 8)
+	format = OFFSET_FORMAT_D;
+
+      else if (msize == 8)
+	format = TARGET_POWERPC64 ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
+
+      else if (TARGET_QUAD_MEMORY)
+	format = OFFSET_FORMAT_DQ;
+
+      else
+	format = TARGET_POWERPC64 ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
+
+      break;
+
+      /* LFD, LFS are d-form, vector types are dq-form.  */
+    case FPR_REG_TYPE:
+      format = (msize < 16) ? OFFSET_FORMAT_D : OFFSET_FORMAT_DQ;
+      break;
+
+      /* LXSSD, LXSD, etc. are ds-form, vector types are dq-form.  */
+    case ALTIVEC_REG_TYPE:
+      format = (msize < 16) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_DQ;
+      break;
+
+      /* LFD, LFS are d-form, LXSSD, LXSD are ds-form, and vector types are
+	 dq-form.  */
+    case VSX_REG_TYPE:
+      if (msize >= 16)
+	format = OFFSET_FORMAT_DQ;
+
+      else if (altivec_reg)
+	format = OFFSET_FORMAT_DS;
+
+      else
+	format = OFFSET_FORMAT_D;
+      break;
+
+      /* SPR, CRs don't have offsettable memory.  */
+    default:
+    case SPR_REG_TYPE:
+    case CR_REG_TYPE:
+      gcc_unreachable ();
+    }
+
+  return format;
+}
 \f
 #if defined (HAVE_GAS_HIDDEN) && !TARGET_MACHO
 /* Emit an assembler directive to set symbol visibility for DECL to

-- 
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], PowerPC, Patch #9, Refine calculation of whether an address offset is d-form, ds-form, or dq-form

[Segher Boessenkool]

On Thu, Jul 11, 2019 at 03:44:02PM -0400, Michael Meissner wrote:
> --- gcc/config/rs6000/rs6000-protos.h	(revision 273371)
> +++ gcc/config/rs6000/rs6000-protos.h	(working copy)
> @@ -154,6 +154,18 @@ extern align_flags rs6000_loop_align (rt
>  extern void rs6000_split_logical (rtx [], enum rtx_code, bool, bool, bool);
>  extern bool rs6000_pcrel_p (struct function *);
>  extern bool rs6000_fndecl_pcrel_p (const_tree);
> +
> +/* Enumeration to describe the offsettable address formats of PowerPC memory
> +   instructions.  */
> +enum rs6000_offset_format {
> +  OFFSET_FORMAT_NONE,		/* No offset format is available.  */
> +  OFFSET_FORMAT_D,		/* All 16-bits are available.  */
> +  OFFSET_FORMAT_DS,		/* Bottom 2 bits must be 0.  */
> +  OFFSET_FORMAT_DQ		/* Bottom 4 bits must be 0.  */
> +};

It's called "instruction form", let's not call it "address format".
"Form" is shorter and more correct, as well.

> +/* Helper function to return whether a MODE can do prefixed loads/stores.
> +   VOIDmode is used when we are loading the pc-relative address into a base
> +   register, but we are not using it as part of a memory operation.  As modes
> +   add support for prefixed memory, they will be added here.  */
> +
> +static bool
> +mode_supports_prefixed_address_p (machine_mode mode)
> +{
> +  return mode == VOIDmode;
> +}

s/Helper function to return/Return/

It's not obvious from this comment that "mode" is the mode of the thing
that is loaded or stored (and not the address of the datum, or something).
The VOIDmode comment wants to say that you use VOIDmode with this function
when you aren't actually loading or storing anything, just taking an
address?

The last line shouldn't be here: predictions will never be true ;-)

What qualifies as a "prefixed load/store" for this?

> +/* Initiailize the table of offset address formats.  Unfortunately we do not

(Typo)

> +  /* Set offset format for some types based on the switches.  */

Many of those are not clear why you allow or do not allow certain forms,
or what each is meant to be used for.  For example:

> +  enum rs6000_offset_format format_gpr_64bit
> +    = (TARGET_POWERPC64) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;

Why does non-powerpc64 allow D?  It cannot even do 64-bit GPR accesses
*at all*!

> +  enum rs6000_offset_format format_gpr_128bit
> +    = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;

128-bit GPRs do not exist, either.

> +  enum rs6000_offset_format format_fpr_64bit
> +    = (TARGET_SOFT_FLOAT) ? OFFSET_FORMAT_D : format_gpr_64bit;

Why does soft float allow more?  Is that even tested?

> +  enum rs6000_offset_format format_vector
> +    = (TARGET_P9_VECTOR) ? OFFSET_FORMAT_DQ : OFFSET_FORMAT_NONE;

Erm, what?  This could use a comment.  Many insns are DS btw, not DQ.

> +  enum rs6000_offset_format format_tf
> +    = (TARGET_IEEEQUAD) ? format_vector : format_fpr_64bit;

TF is either IF or KF.

> +  /* Small integers.  */
> +  reg_addr[E_QImode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_HImode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_CQImode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_CHImode].offset_format = OFFSET_FORMAT_D;

Do we handle CQI and CHI anywhere else?

> +  /* While DImode can be loaded into a FPR register with LFD (d-form), the
> +     normal use is to use a GPR (ds-form in 64-bit).  */
> +  reg_addr[E_DImode].offset_format = format_gpr_64bit;
> +  reg_addr[E_CDImode].offset_format = format_gpr_64bit;

I'm not sure that comment helps anything -- I don't know what it is trying
to say?

> +  /* Condition registers (uses LWZ/STW).  */
> +  reg_addr[E_CCmode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_CCUNSmode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_CCFPmode].offset_format = OFFSET_FORMAT_D;
> +  reg_addr[E_CCEQmode].offset_format = OFFSET_FORMAT_D;

Okay, why do we make the "allowed address form" function support modes
that we have no load/store insns for?!

> +  /* 128-bit integers.  */
> +  if (TARGET_POWERPC64)
> +    {
> +      reg_addr[E_TImode].offset_format = format_vector;

TI is not normally / always / often in vectors.

So what is this really doing?  Preferred form for accesses in a certain
mode?

> +  /* Initial the mapping of mode to offset formats.  */
> +  initialize_offset_formats ();

That comment is redundant.  (Also, spelling).

> +  /* Is this a valid prefixed address?  If the bottom four bits of the offset
> +     are non-zero, we could use a prefixed instruction (which does not have the
> +     DQ-form constraint that the traditional instruction had) instead of
> +     forcing the unaligned offset to a GPR.  */
> +  if (mode_supports_prefixed_address_p (mode)
> +      && rs6000_prefixed_address_format (addr, OFFSET_FORMAT_DQ))
> +    return true;

The comment says something different than the code does?

> +      switch (format)
> +	{
> +	default:
> +	  gcc_unreachable ();

First not but the end at default go should.

> +/* Return true if ADDR is a valid prefixed memory address that uses mode
> +   MODE.  */
> +
> +bool
> +rs6000_prefixed_address_mode (rtx addr, machine_mode mode)

Needs _p or *_is_* or whatever.  The name should not suggest it returns
a mode, since it doesn't.


Please change format to form, and change all names and comments so it is
clear what things do, and that it makes *sense*.  It might well work, but
I cannot make heads or tails of it.  Maybe if I would spend another day
on it?


Segher

Re: [PATCH], PowerPC, Patch #9, Refine calculation of whether an address offset is d-form, ds-form, or dq-form

[Michael Meissner]

On Fri, Jul 12, 2019 at 11:49:51AM -0500, Segher Boessenkool wrote:
> Many of those are not clear why you allow or do not allow certain forms,
> or what each is meant to be used for.  For example:
> 
> > +  enum rs6000_offset_format format_gpr_64bit
> > +    = (TARGET_POWERPC64) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
> 
> Why does non-powerpc64 allow D?  It cannot even do 64-bit GPR accesses
> *at all*!
> 

A single instruction cannot do 64-bit GPR loads, BUT the tests are needed for
register allocation before the instruction is split.  So in 32-bit mode, load
of DImode is split into 2 loads of SImode, which is D-format.

> > +  enum rs6000_offset_format format_gpr_128bit
> > +    = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;
> 
> 128-bit GPRs do not exist, either.

Yes they do during register allocation, and are split later before final.

> > +  enum rs6000_offset_format format_fpr_64bit
> > +    = (TARGET_SOFT_FLOAT) ? OFFSET_FORMAT_D : format_gpr_64bit;
> 
> Why does soft float allow more?  Is that even tested?

Because on a 32-bit system, soft float DFmode goes into 2 GPRs that are D-format.
On a 64-bit system, soft float DFmode goes into 1 GPR that is DS-format.

> > +  enum rs6000_offset_format format_vector
> > +    = (TARGET_P9_VECTOR) ? OFFSET_FORMAT_DQ : OFFSET_FORMAT_NONE;
> 
> Erm, what?  This could use a comment.  Many insns are DS btw, not DQ.
> 
> > +  enum rs6000_offset_format format_tf
> > +    = (TARGET_IEEEQUAD) ? format_vector : format_fpr_64bit;
> 
> TF is either IF or KF.

Yes, and when TF is KFmode, it is a vector (DQ-format), but when it is IFmode,
it is a pair of DFmode values (i.e. typically D-format, but it can be DS-format
when loading into the traditional Altivec registers.

This is in part why I think the mode format is broken.  But, until register
allocation, we have to guess and that is what this function is trying to do.
It is used by the legitimate address functions and the constraints.

But then I've felt that the current legitimate address stuff has been broken
ever since I started working on GCC 1.37 because you don't have the context of
whether it is a load or store, and don't have the register loaded to or stored
after register allocation.  But it is what it is.

> 
> > +  /* Small integers.  */
> > +  reg_addr[E_QImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_HImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CQImode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CHImode].offset_format = OFFSET_FORMAT_D;
> 
> Do we handle CQI and CHI anywhere else?

It depends.  It may use this format early in the RTL generation until the
complex types are split.

> 
> > +  /* While DImode can be loaded into a FPR register with LFD (d-form), the
> > +     normal use is to use a GPR (ds-form in 64-bit).  */
> > +  reg_addr[E_DImode].offset_format = format_gpr_64bit;
> > +  reg_addr[E_CDImode].offset_format = format_gpr_64bit;
> 
> I'm not sure that comment helps anything -- I don't know what it is trying
> to say?

Normally when you use a DImode, it goes into a GPR (i.e. DS-format in 64-bit).
However, there are times when you want to use a DImode in a FPR, when you can
use D-format.  As an example, the test dimode_off.c hand crafts loads and
stores with odd offsets.  We had to de-tune movdi so that the register
allocator would not load the value into a FPR and do a direct move (or do a
direct move and store from the FPR).

> > +  /* Condition registers (uses LWZ/STW).  */
> > +  reg_addr[E_CCmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCUNSmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCFPmode].offset_format = OFFSET_FORMAT_D;
> > +  reg_addr[E_CCEQmode].offset_format = OFFSET_FORMAT_D;
> 
> Okay, why do we make the "allowed address form" function support modes
> that we have no load/store insns for?!

But we do have load and store functions for the CC* modes.  See movcc_internal1.

> > +  /* 128-bit integers.  */
> > +  if (TARGET_POWERPC64)
> > +    {
> > +      reg_addr[E_TImode].offset_format = format_vector;
> 
> TI is not normally / always / often in vectors.

Not now, but it will be.

> So what is this really doing?  Preferred form for accesses in a certain
> mode?

Basically the whole thing is a guess of what the valid offset format for a mode
is.  This is needed for the legitimate address functions
(i.e. rs6000_legitimate_address_p, rs6000_legitimate_offset_address_p) to
validate whether we allow odd addresses or not.

> > +  /* Initial the mapping of mode to offset formats.  */
> > +  initialize_offset_formats ();
> 
> That comment is redundant.  (Also, spelling).
> 
> > +  /* Is this a valid prefixed address?  If the bottom four bits of the offset
> > +     are non-zero, we could use a prefixed instruction (which does not have the
> > +     DQ-form constraint that the traditional instruction had) instead of
> > +     forcing the unaligned offset to a GPR.  */
> > +  if (mode_supports_prefixed_address_p (mode)
> > +      && rs6000_prefixed_address_format (addr, OFFSET_FORMAT_DQ))
> > +    return true;
> 
> The comment says something different than the code does?
> 
> > +      switch (format)
> > +	{
> > +	default:
> > +	  gcc_unreachable ();
> 
> First not but the end at default go should.
> 
> > +/* Return true if ADDR is a valid prefixed memory address that uses mode
> > +   MODE.  */
> > +
> > +bool
> > +rs6000_prefixed_address_mode (rtx addr, machine_mode mode)
> 
> Needs _p or *_is_* or whatever.  The name should not suggest it returns
> a mode, since it doesn't.
> 
> 
> Please change format to form, and change all names and comments so it is
> clear what things do, and that it makes *sense*.  It might well work, but
> I cannot make heads or tails of it.  Maybe if I would spend another day
> on it?
> 
> 
> 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], PowerPC, Patch #9, Refine calculation of whether an address offset is d-form, ds-form, or dq-form

[Segher Boessenkool]

%s/format/form/g

On Tue, Jul 16, 2019 at 01:29:50PM -0400, Michael Meissner wrote:
> On Fri, Jul 12, 2019 at 11:49:51AM -0500, Segher Boessenkool wrote:
> > Many of those are not clear why you allow or do not allow certain forms,
> > or what each is meant to be used for.  For example:
> > 
> > > +  enum rs6000_offset_format format_gpr_64bit
> > > +    = (TARGET_POWERPC64) ? OFFSET_FORMAT_DS : OFFSET_FORMAT_D;
> > 
> > Why does non-powerpc64 allow D?  It cannot even do 64-bit GPR accesses
> > *at all*!
> 
> A single instruction cannot do 64-bit GPR loads, BUT the tests are needed for
> register allocation before the instruction is split.  So in 32-bit mode, load
> of DImode is split into 2 loads of SImode, which is D-format.

Then the variable is misnamed?

> > > +  enum rs6000_offset_format format_gpr_128bit
> > > +    = (TARGET_QUAD_MEMORY) ? OFFSET_FORMAT_DQ : format_gpr_64bit;
> > 
> > 128-bit GPRs do not exist, either.
> 
> Yes they do during register allocation, and are split later before final.

You mean that you have a TImode datum in two GPRs.  There are no 128-bit
GPRs.

The *two* GPRs should be part of the name, that would improve it?

> This is in part why I think the mode format is broken.

The alternative is picking at expand time what register set to use where.
That is worse.  We want to use vectors often, but certainly not always.

> But then I've felt that the current legitimate address stuff has been broken
> ever since I started working on GCC 1.37 because you don't have the context of
> whether it is a load or store, and don't have the register loaded to or stored
> after register allocation.  But it is what it is.

That is one of reload's big jobs.  If you have a better design...  :-)

> > > +  /* Small integers.  */
> > > +  reg_addr[E_QImode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_HImode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_CQImode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_CHImode].offset_format = OFFSET_FORMAT_D;
> > 
> > Do we handle CQI and CHI anywhere else?
> 
> It depends.  It may use this format early in the RTL generation until the
> complex types are split.

Is that used *during* expand?  Blergh.

> > > +  /* While DImode can be loaded into a FPR register with LFD (d-form), the
> > > +     normal use is to use a GPR (ds-form in 64-bit).  */
> > > +  reg_addr[E_DImode].offset_format = format_gpr_64bit;
> > > +  reg_addr[E_CDImode].offset_format = format_gpr_64bit;
> > 
> > I'm not sure that comment helps anything -- I don't know what it is trying
> > to say?
> 
> Normally when you use a DImode, it goes into a GPR (i.e. DS-format in 64-bit).
> However, there are times when you want to use a DImode in a FPR, when you can
> use D-format.  As an example, the test dimode_off.c hand crafts loads and
> stores with odd offsets.  We had to de-tune movdi so that the register
> allocator would not load the value into a FPR and do a direct move (or do a
> direct move and store from the FPR).

That doesn't explain the comment to me, sorry :-/

> > > +  /* Condition registers (uses LWZ/STW).  */
> > > +  reg_addr[E_CCmode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_CCUNSmode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_CCFPmode].offset_format = OFFSET_FORMAT_D;
> > > +  reg_addr[E_CCEQmode].offset_format = OFFSET_FORMAT_D;
> > 
> > Okay, why do we make the "allowed address form" function support modes
> > that we have no load/store insns for?!
> 
> But we do have load and store functions for the CC* modes.  See movcc_internal1.

I meant machine instructions.

movcc_internal has nothing to load/store anything in a CC reg from/to
memory, either.

Do we really have to care what might happen *after* we put something in
a GPR?

> > > +  /* 128-bit integers.  */
> > > +  if (TARGET_POWERPC64)
> > > +    {
> > > +      reg_addr[E_TImode].offset_format = format_vector;
> > 
> > TI is not normally / always / often in vectors.
> 
> Not now, but it will be.

1) Erm.
2) No, you cannot break p8 and p9 performance like this.

> Basically the whole thing is a guess of what the valid offset format for a mode
> is.  This is needed for the legitimate address functions
> (i.e. rs6000_legitimate_address_p, rs6000_legitimate_offset_address_p) to
> validate whether we allow odd addresses or not.

This needs to be made much more explicit, then.  "preferred_form" or
something.

Then suddenly it will probably start to make sense :-)


Segher

[PATCH], Patch #6, revision 3, Create pc-relative addressing insns

[Michael Meissner]

This is a re-think of patch #6.

I have changed the TARGET_TOC to be TARGET_HAS_TOC in the aix, darwin, system
V, and Linux 64-bit headers.  Then in rs6000.h, TARGET_TOC is defined in terms
of TARGET_HAS_TOC and not pc-relative referencing.

I discovered that TARGET_NO_TOC must not be set to be just !TARGET_TOC, since
TARGET_NO_TOC is used to create the elf_high, elf_low insns in 32-bit.

I added a gcc_assert in create_TOC_reference.

For pc-relative, I dropped setting an alias set, since it uses the constant
pool SYMBOL_REF created by force_const_mem (the TOC code needs to create the
alias set because it rewrites the memory address, and the constant pool stuff
is missing).  Note, there is some more code that will need to be done when
pc-relative support is completely supported, and that will be in a later patch.

I did rename the static variable 'set' that contained the alias set to
TOC_alias_set.  I did not move the initialization of the TOC_alias_set
elsewhere, because in order to call TOC_alias_set, the code has already called
force_const_mem, create_TOC_reference, and gen_const_mem, so I didn't see the
point of adding a micro-optimization for this.

For that future pc-relative change, I did add code to only allow pc-relative if
the memory model is medium.  This eliminates some of the checks you objected to
in the previous patch.

I did build the compiler on both big endian and little endian power8 system,
and ran the regression tests (both 64/32-bit on the big endian system, and just
64-bit on the little endian system) and there were no regressions.

In addition, I built spec 2017 and spec 2006 for both little endian power9 and
the future system using the branch that contains the full future pc-relative
changes that I'm submitting bits and pieces from.  I reworked that branch to
use the same patches that are being submitted.

Can I check this into the trunk?

2019-07-15  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/aix.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define in rs6000.h.
	* config/rs6000/darwin.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define in rs6000.h.
	* config/rs6000/linux64.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Add
	check to require -mcmodel=medium for pc-relative addressing.
	(create_TOC_reference): Add assertion for TARGET_TOC.
	(TOC_alias_set): Rename TOC alias set static variable from 'set'
	to 'TOC_alias_set'.
	(get_TOC_alias_set): Likewise.
	* config/rs6000/rs6000.h (TARGET_TOC): Define in terms of
	TARGET_HAS_TOC and not pc-relative.
	(TARGET_NO_TOC): Likewise.
	* config/rs6000/sysv4.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define in rs6000.h.

Index: gcc/config/rs6000/aix.h
===================================================================
--- gcc/config/rs6000/aix.h	(revision 273457)
+++ gcc/config/rs6000/aix.h	(working copy)
@@ -32,8 +32,7 @@
 #define TARGET_AIX_OS 1
 
 /* AIX always has a TOC.  */
-#define TARGET_NO_TOC 0
-#define TARGET_TOC 1
+#define TARGET_HAS_TOC 1
 #define FIXED_R2 1
 
 /* AIX allows r13 to be used in 32-bit mode.  */
Index: gcc/config/rs6000/darwin.h
===================================================================
--- gcc/config/rs6000/darwin.h	(revision 273457)
+++ gcc/config/rs6000/darwin.h	(working copy)
@@ -43,8 +43,7 @@
 
 /* We're not ever going to do TOCs.  */
 
-#define TARGET_TOC 0
-#define TARGET_NO_TOC 1
+#define TARGET_HAS_TOC 0
 
 /* Override the default rs6000 definition.  */
 #undef  PTRDIFF_TYPE
Index: gcc/config/rs6000/linux64.h
===================================================================
--- gcc/config/rs6000/linux64.h	(revision 273457)
+++ gcc/config/rs6000/linux64.h	(working copy)
@@ -277,8 +277,8 @@ extern int dot_symbols;
 #ifndef RS6000_BI_ARCH
 
 /* 64-bit PowerPC Linux always has a TOC.  */
-#undef  TARGET_TOC
-#define	TARGET_TOC		1
+#undef  TARGET_HAS_TOC
+#define	TARGET_HAS_TOC		1
 
 /* Some things from sysv4.h we don't do when 64 bit.  */
 #undef	OPTION_RELOCATABLE
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273457)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4333,6 +4333,16 @@ 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)
+    {
+      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
+	error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
+
+      rs6000_isa_flags &= ~OPTION_MASK_PCREL;
+    }
+
   if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
     rs6000_print_isa_options (stderr, 0, "after subtarget", rs6000_isa_flags);
 
@@ -7744,6 +7754,8 @@ create_TOC_reference (rtx symbol, rtx la
 {
   rtx tocrel, tocreg, hi;
 
+  gcc_assert (TARGET_TOC);
+
   if (TARGET_DEBUG_ADDR)
     {
       if (SYMBOL_REF_P (symbol))
@@ -23736,14 +23748,14 @@ rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+static GTY(()) alias_set_type TOC_alias_set = -1;
 
 alias_set_type
 get_TOC_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (TOC_alias_set == -1)
+    TOC_alias_set = new_alias_set ();
+  return TOC_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 273457)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -54,6 +54,14 @@
 #define TARGET_AIX_OS 0
 #endif
 
+/* Turn off TOC support if pc-relative addressing is used.  However, do not
+   turn on TARGET_NO_TOC if we have pc-relative addressing.  The places that
+   check TARGET_NO_TOC are mostly for 32-bit ELF systems using elf_high and
+   elf_low, and we do not want to generate those instructions if we have
+   pc-relative support.  */
+#define TARGET_TOC    (TARGET_HAS_TOC && !TARGET_PCREL)
+#define TARGET_NO_TOC (!TARGET_HAS_TOC && !TARGET_PCREL)
+
 /* Control whether function entry points use a "dot" symbol when
    ABI_AIX.  */
 #define DOT_SYMBOLS 1
Index: gcc/config/rs6000/sysv4.h
===================================================================
--- gcc/config/rs6000/sysv4.h	(revision 273457)
+++ gcc/config/rs6000/sysv4.h	(working copy)
@@ -41,7 +41,7 @@
 #undef	ASM_DEFAULT_SPEC
 #define	ASM_DEFAULT_SPEC "-mppc"
 
-#define	TARGET_TOC		(TARGET_64BIT				\
+#define	TARGET_HAS_TOC		(TARGET_64BIT				\
 				 || (TARGET_MINIMAL_TOC			\
 				     && flag_pic > 1)			\
 				 || DEFAULT_ABI != ABI_V4)
@@ -50,7 +50,6 @@
 #define	TARGET_BIG_ENDIAN	(! TARGET_LITTLE_ENDIAN)
 #define	TARGET_PROTOTYPE	target_prototype
 #define	TARGET_NO_PROTOTYPE	(! TARGET_PROTOTYPE)
-#define	TARGET_NO_TOC		(! TARGET_TOC)
 #define	TARGET_NO_EABI		(! TARGET_EABI)
 #define	TARGET_REGNAMES		rs6000_regnames
 

-- 
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], Patch #6, revision 3, Create pc-relative addressing insns

[Segher Boessenkool]

Hi Mike,

On Tue, Jul 16, 2019 at 02:19:14AM -0400, Michael Meissner wrote:
> I have changed the TARGET_TOC to be TARGET_HAS_TOC in the aix, darwin, system
> V, and Linux 64-bit headers.  Then in rs6000.h, TARGET_TOC is defined in terms
> of TARGET_HAS_TOC and not pc-relative referencing.

Cool, thanks.  Good name, too.

> I discovered that TARGET_NO_TOC must not be set to be just !TARGET_TOC, since
> TARGET_NO_TOC is used to create the elf_high, elf_low insns in 32-bit.

I don't know if your setting in sysv4.h works.  This file is used on so
very many platforms, it is hard to predict if it works everywhere :-/

> I did rename the static variable 'set' that contained the alias set to
> TOC_alias_set.

:-)

> I did not move the initialization of the TOC_alias_set
> elsewhere, because in order to call TOC_alias_set, the code has already called
> force_const_mem, create_TOC_reference, and gen_const_mem, so I didn't see the
> point of adding a micro-optimization for this.

It gets rid of a call, but also of the conditional, and that makes this
eminently inlinable.  You could remove the getter function completely
even, access the variable directly.

But, sure, that's existing code only now.

> Index: gcc/config/rs6000/linux64.h
> ===================================================================
> --- gcc/config/rs6000/linux64.h	(revision 273457)
> +++ gcc/config/rs6000/linux64.h	(working copy)
> @@ -277,8 +277,8 @@ extern int dot_symbols;
>  #ifndef RS6000_BI_ARCH
>  
>  /* 64-bit PowerPC Linux always has a TOC.  */
> -#undef  TARGET_TOC
> -#define	TARGET_TOC		1
> +#undef  TARGET_HAS_TOC
> +#define	TARGET_HAS_TOC		1

Fix the tab while your at it?  Not that it is consistent at all in this
file, but having the undef and the define in different style... :-)


So, what does TARGET_NO_TOC mean now?  Maybe a better name would help,
or some documentation if not?

Looks good otherwise, okay for trunk.  Thanks!

(And watch out if it works on AIX and Darwin, please).


Segher

Re: [PATCH], Patch #6, revision 3, Create pc-relative addressing insns

[Michael Meissner]

On Tue, Jul 16, 2019 at 03:58:18PM -0500, Segher Boessenkool wrote:
> Hi Mike,
> 
> On Tue, Jul 16, 2019 at 02:19:14AM -0400, Michael Meissner wrote:
> > I have changed the TARGET_TOC to be TARGET_HAS_TOC in the aix, darwin, system
> > V, and Linux 64-bit headers.  Then in rs6000.h, TARGET_TOC is defined in terms
> > of TARGET_HAS_TOC and not pc-relative referencing.
> 
> Cool, thanks.  Good name, too.
> 
> > I discovered that TARGET_NO_TOC must not be set to be just !TARGET_TOC, since
> > TARGET_NO_TOC is used to create the elf_high, elf_low insns in 32-bit.
> 
> I don't know if your setting in sysv4.h works.  This file is used on so
> very many platforms, it is hard to predict if it works everywhere :-/

Well it is a mechanical change.

> > I did rename the static variable 'set' that contained the alias set to
> > TOC_alias_set.
> 
> :-)
> 
> > I did not move the initialization of the TOC_alias_set
> > elsewhere, because in order to call TOC_alias_set, the code has already called
> > force_const_mem, create_TOC_reference, and gen_const_mem, so I didn't see the
> > point of adding a micro-optimization for this.
> 
> It gets rid of a call, but also of the conditional, and that makes this
> eminently inlinable.  You could remove the getter function completely
> even, access the variable directly.
> 
> But, sure, that's existing code only now.

We can fix this later.

> > Index: gcc/config/rs6000/linux64.h
> > ===================================================================
> > --- gcc/config/rs6000/linux64.h	(revision 273457)
> > +++ gcc/config/rs6000/linux64.h	(working copy)
> > @@ -277,8 +277,8 @@ extern int dot_symbols;
> >  #ifndef RS6000_BI_ARCH
> >  
> >  /* 64-bit PowerPC Linux always has a TOC.  */
> > -#undef  TARGET_TOC
> > -#define	TARGET_TOC		1
> > +#undef  TARGET_HAS_TOC
> > +#define	TARGET_HAS_TOC		1
> 
> Fix the tab while your at it?  Not that it is consistent at all in this
> file, but having the undef and the define in different style... :-)
> 
> 
> So, what does TARGET_NO_TOC mean now?  Maybe a better name would help,
> or some documentation if not?
> 
> Looks good otherwise, okay for trunk.  Thanks!
> 
> (And watch out if it works on AIX and Darwin, please).

David has said the earlier patch works on AIX, and Ian has said he will test it
when he is able to.

I did rename the TARGET_NO_TOC macro to TARGET_NO_TOC_OR_PCREL which hopefully
makes it more obvious when it should be true.

Here is the patch I committed:

2019-07-18  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/aix.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define TARGET_NO_TOC_OR_PCREL in
	rs6000.h.
	* config/rs6000/darwin.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define TARGET_NO_TOC_OR_PCREL in
	rs6000.h.
	* config/rs6000/linux64.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Add
	check to require -mcmodel=medium for pc-relative addressing.
	(create_TOC_reference): Add assertion for TARGET_TOC.
	(rs6000_legitimize_address): Use TARGET_NO_TOC_OR_PCREL instead of
	TARGET_NO_TOC.
	(rs6000_emit_move): Likewise.
	(TOC_alias_set): Rename TOC alias set static variable from 'set'
	to 'TOC_alias_set'.
	(get_TOC_alias_set): Likewise.
	(output_toc): Use TARGET_NO_TOC_OR_PCREL instead of
	TARGET_NO_TOC.
	(rs6000_can_eliminate): Likewise.
	* config/rs6000/rs6000.h (TARGET_TOC): Define in terms of
	TARGET_HAS_TOC and not pc-relative.
	(TARGET_NO_TOC_OR_PCREL): New macro to replace TARGET_NO_TOC.
	* config/rs6000/sysv4.h (TARGET_HAS_TOC): Rename TARGET_TOC to
	TARGET_HAS_TOC.
	(TARGET_TOC): Likewise.
	(TARGET_NO_TOC): Delete here, define TARGET_NO_TOC_OR_PCREL in
	rs6000.h.

Index: gcc/config/rs6000/aix.h
===================================================================
--- gcc/config/rs6000/aix.h	(revision 273578)
+++ gcc/config/rs6000/aix.h	(working copy)
@@ -32,8 +32,7 @@
 #define TARGET_AIX_OS 1
 
 /* AIX always has a TOC.  */
-#define TARGET_NO_TOC 0
-#define TARGET_TOC 1
+#define TARGET_HAS_TOC 1
 #define FIXED_R2 1
 
 /* AIX allows r13 to be used in 32-bit mode.  */
Index: gcc/config/rs6000/darwin.h
===================================================================
--- gcc/config/rs6000/darwin.h	(revision 273578)
+++ gcc/config/rs6000/darwin.h	(working copy)
@@ -43,8 +43,7 @@
 
 /* We're not ever going to do TOCs.  */
 
-#define TARGET_TOC 0
-#define TARGET_NO_TOC 1
+#define TARGET_HAS_TOC 0
 
 /* Override the default rs6000 definition.  */
 #undef  PTRDIFF_TYPE
Index: gcc/config/rs6000/linux64.h
===================================================================
--- gcc/config/rs6000/linux64.h	(revision 273578)
+++ gcc/config/rs6000/linux64.h	(working copy)
@@ -277,8 +277,8 @@ extern int dot_symbols;
 #ifndef RS6000_BI_ARCH
 
 /* 64-bit PowerPC Linux always has a TOC.  */
-#undef  TARGET_TOC
-#define	TARGET_TOC		1
+#undef  TARGET_HAS_TOC
+#define TARGET_HAS_TOC		1
 
 /* Some things from sysv4.h we don't do when 64 bit.  */
 #undef	OPTION_RELOCATABLE
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273578)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4333,6 +4333,16 @@ 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)
+    {
+      if ((rs6000_isa_flags_explicit & OPTION_MASK_PCREL) != 0)
+	error ("%qs requires %qs", "-mpcrel", "-mcmodel=medium");
+
+      rs6000_isa_flags &= ~OPTION_MASK_PCREL;
+    }
+
   if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
     rs6000_print_isa_options (stderr, 0, "after subtarget", rs6000_isa_flags);
 
@@ -7742,6 +7752,8 @@ create_TOC_reference (rtx symbol, rtx la
 {
   rtx tocrel, tocreg, hi;
 
+  gcc_assert (TARGET_TOC);
+
   if (TARGET_DEBUG_ADDR)
     {
       if (SYMBOL_REF_P (symbol))
@@ -8121,7 +8133,7 @@ rs6000_legitimize_address (rtx x, rtx ol
 #endif
 	    )
 	   && TARGET_32BIT
-	   && TARGET_NO_TOC
+	   && TARGET_NO_TOC_OR_PCREL
 	   && !flag_pic
 	   && !CONST_INT_P (x)
 	   && !CONST_WIDE_INT_P (x)
@@ -9811,7 +9823,7 @@ rs6000_emit_move (rtx dest, rtx source,
 	}
 
       if ((TARGET_ELF || DEFAULT_ABI == ABI_DARWIN)
-	  && TARGET_NO_TOC
+	  && TARGET_NO_TOC_OR_PCREL
 	  && ! flag_pic
 	  && mode == Pmode
 	  && CONSTANT_P (operands[1])
@@ -23725,14 +23737,14 @@ rs6000_split_multireg_move (rtx dst, rtx
     }
 }
 
-static GTY(()) alias_set_type set = -1;
+static GTY(()) alias_set_type TOC_alias_set = -1;
 
 alias_set_type
 get_TOC_alias_set (void)
 {
-  if (set == -1)
-    set = new_alias_set ();
-  return set;
+  if (TOC_alias_set == -1)
+    TOC_alias_set = new_alias_set ();
+  return TOC_alias_set;
 }
 
 /* Return the internal arg pointer used for function incoming
@@ -24103,7 +24115,7 @@ output_toc (FILE *file, rtx x, int label
   rtx base = x;
   HOST_WIDE_INT offset = 0;
 
-  gcc_assert (!TARGET_NO_TOC);
+  gcc_assert (!TARGET_NO_TOC_OR_PCREL);
 
   /* When the linker won't eliminate them, don't output duplicate
      TOC entries (this happens on AIX if there is any kind of TOC,
@@ -30469,7 +30481,7 @@ rs6000_can_eliminate (const int from, co
   return (from == ARG_POINTER_REGNUM && to == STACK_POINTER_REGNUM
 	  ? ! frame_pointer_needed
 	  : from == RS6000_PIC_OFFSET_TABLE_REGNUM
-	    ? ! TARGET_MINIMAL_TOC || TARGET_NO_TOC
+	    ? ! TARGET_MINIMAL_TOC || TARGET_NO_TOC_OR_PCREL
 		|| constant_pool_empty_p ()
 	    : true);
 }
Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 273578)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -54,6 +54,13 @@
 #define TARGET_AIX_OS 0
 #endif
 
+/* Turn off TOC support if pc-relative addressing is used.  */
+#define TARGET_TOC             (TARGET_HAS_TOC && !TARGET_PCREL)
+
+/* On 32-bit systems without a TOC or pc-relative addressing, we need to use
+   ADDIS/ADDI to load up the address of a symbol.  */
+#define TARGET_NO_TOC_OR_PCREL (!TARGET_HAS_TOC && !TARGET_PCREL)
+
 /* Control whether function entry points use a "dot" symbol when
    ABI_AIX.  */
 #define DOT_SYMBOLS 1
Index: gcc/config/rs6000/sysv4.h
===================================================================
--- gcc/config/rs6000/sysv4.h	(revision 273578)
+++ gcc/config/rs6000/sysv4.h	(working copy)
@@ -41,7 +41,7 @@
 #undef	ASM_DEFAULT_SPEC
 #define	ASM_DEFAULT_SPEC "-mppc"
 
-#define	TARGET_TOC		(TARGET_64BIT				\
+#define	TARGET_HAS_TOC		(TARGET_64BIT				\
 				 || (TARGET_MINIMAL_TOC			\
 				     && flag_pic > 1)			\
 				 || DEFAULT_ABI != ABI_V4)
@@ -50,7 +50,6 @@
 #define	TARGET_BIG_ENDIAN	(! TARGET_LITTLE_ENDIAN)
 #define	TARGET_PROTOTYPE	target_prototype
 #define	TARGET_NO_PROTOTYPE	(! TARGET_PROTOTYPE)
-#define	TARGET_NO_TOC		(! TARGET_TOC)
 #define	TARGET_NO_EABI		(! TARGET_EABI)
 #define	TARGET_REGNAMES		rs6000_regnames
 

-- 
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], Patch #6, revision 3, Create pc-relative addressing insns

[Segher Boessenkool]

On Thu, Jul 18, 2019 at 02:20:43PM -0400, Michael Meissner wrote:
> On Tue, Jul 16, 2019 at 03:58:18PM -0500, Segher Boessenkool wrote:
> > > I did not move the initialization of the TOC_alias_set
> > > elsewhere, because in order to call TOC_alias_set, the code has already called
> > > force_const_mem, create_TOC_reference, and gen_const_mem, so I didn't see the
> > > point of adding a micro-optimization for this.
> > 
> > It gets rid of a call, but also of the conditional, and that makes this
> > eminently inlinable.  You could remove the getter function completely
> > even, access the variable directly.
> > 
> > But, sure, that's existing code only now.
> 
> We can fix this later.

Yup :-)

> I did rename the TARGET_NO_TOC macro to TARGET_NO_TOC_OR_PCREL which hopefully
> makes it more obvious when it should be true.

It does, thanks!


Segher

Re: [PATCH], Patch #6, revision 3, (Test on AIX and Darwin)

[Michael Meissner]

David, would it be possible to do a bootstrap on AIX to make sure I haven't
broken anything before I commit the patch to the trunk?

Ian, if you have time, could you also do the PowerPC Darwin port that would be
helpful also.

Now, the changes are pretty simple, and I expect them to be fine.  Basically
for the new 'future' machine, we have pc-relative addressing, and we want to
turn off the TOC suport.

The patch is on the SVN branch:
	svn+ssh://gcc.gnu.org/svn/gcc/branches/ibm/pcrel-trunk

And I just merged the branch up to trunk top earlier today.

The patch in question is in this post:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg01080.html

Thanks in advance.

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Michael Meissner]

I will be iterating on patch #9 and sending out a replacement shortly.

This is patch #10.  It moves the various data structures from rs6000.c to
rs6000-internal.h.  In the future, it will allow us to move more things from
rs6000.c to other files.  In particular, I plan on adding a rs6000-prefixed.c
file that has the specific support for prefixed instructions.

It would also allow various functions for handling adressing and register
assignment, etc. that could also be moved to a separate file as well.

This patch just moves the data structure definitions and helper functions
(mode_supports_*) to rs6000-internal.h.  The static data structures are changed
to external, and definitions of the function are put in rs6000.c.  I did add 00
to the bit mask definitions for RELOAD_REG_* because I plan to add at least one
more mask (for DS-form addresses) in the future, and it will the values lined
up.

I have done a bootstrap on a little endian power8 system running Linux and
there were no regressions in the test suite.  Can I check this into the trunk?

2019-07-20  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p):
	Move various declarations relating to addressing and register
	allocation to rs6000-internal.h from rs6000.c so that in the
	future we can move things out of rs6000.c.  Make the static arrays
	global, and define them in rs6000.c.
	(enum rs6000_reg_type): Likewise.
	(reg_class_to_reg_type): Likewise.
	(IS_STD_REG_TYPE): Likewise.
	(IS_FP_VECT_REG_TYPE): Likewise.
	(enum rs6000_reload_reg_type): Likewise.
	(FIRST_RELOAD_REG_CLASS): Likewise.
	(LAST_RELOAD_REG_CLASS): Likewise.
	(struct reload_reg_map_type): Likewise.
	(reload_reg_map): Likewise.
	(RELOAD_REG_* macros): Likewise.
	(struct rs6000_reg_addr): Likewise.
	(reg_addr): Likewise.
	(mode_supports_pre_incdec_): Likewise.
	(mode_supports_pre_modify_p): Likewise.
	(mode_supports_vmx_dform): Likewise.
	(mode_supports_dq_form): Likewise.
	(mode_supports_prefixed_address_p): Likewise.
	* config/rs6000/rs6000.c (rs6000_hard_regno_mode_ok_p): Likewise.
	(enum rs6000_reg_type): Likewise.
	(reg_class_to_reg_type): Likewise.
	(IS_STD_REG_TYPE): Likewise.
	(IS_FP_VECT_REG_TYPE): Likewise.
	(enum rs6000_reload_reg_type): Likewise.
	(FIRST_RELOAD_REG_CLASS): Likewise.
	(LAST_RELOAD_REG_CLASS): Likewise.
	(struct reload_reg_map_type): Likewise.
	(reload_reg_map): Likewise.
	(RELOAD_REG_* macros): Likewise.
	(struct rs6000_reg_addr): Likewise.
	(reg_addr): Likewise.
	(mode_supports_pre_incdec_): Likewise.
	(mode_supports_pre_modify_p): Likewise.
	(mode_supports_vmx_dform): Likewise.
	(mode_supports_dq_form): Likewise.
	(mode_supports_prefixed_address_p): Likewise.

Index: gcc/config/rs6000/rs6000-internal.h
===================================================================
--- gcc/config/rs6000/rs6000-internal.h	(revision 273617)
+++ gcc/config/rs6000/rs6000-internal.h	(working copy)
@@ -22,6 +22,134 @@
 #ifndef GCC_RS6000_INTERNAL_H
 #define GCC_RS6000_INTERNAL_H
 
+/* Value is TRUE if register/mode pair is acceptable.  */
+extern bool rs6000_hard_regno_mode_ok_p
+  [NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
+
+/* Simplfy register classes into simpler classifications.  We assume
+   GPR_REG_TYPE - FPR_REG_TYPE are ordered so that we can use a simple range
+   check for standard register classes (gpr/floating/altivec/vsx) and
+   floating/vector classes (float/altivec/vsx).  */
+
+enum rs6000_reg_type {
+  NO_REG_TYPE,
+  PSEUDO_REG_TYPE,
+  GPR_REG_TYPE,
+  VSX_REG_TYPE,
+  ALTIVEC_REG_TYPE,
+  FPR_REG_TYPE,
+  SPR_REG_TYPE,
+  CR_REG_TYPE
+};
+
+/* Map register class to register type.  */
+extern enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
+
+/* First/last register type for the 'normal' register types (i.e. general
+   purpose, floating point, altivec, and VSX registers).  */
+#define IS_STD_REG_TYPE(RTYPE) IN_RANGE(RTYPE, GPR_REG_TYPE, FPR_REG_TYPE)
+
+#define IS_FP_VECT_REG_TYPE(RTYPE) IN_RANGE(RTYPE, VSX_REG_TYPE, FPR_REG_TYPE)
+
+
+/* Register classes we care about in secondary reload or go if legitimate
+   address.  We only need to worry about GPR, FPR, and Altivec registers here,
+   along an ANY field that is the OR of the 3 register classes.  */
+
+enum rs6000_reload_reg_type {
+  RELOAD_REG_GPR,			/* General purpose registers.  */
+  RELOAD_REG_FPR,			/* Traditional floating point regs.  */
+  RELOAD_REG_VMX,			/* Altivec (VMX) registers.  */
+  RELOAD_REG_ANY,			/* OR of GPR, FPR, Altivec masks.  */
+  N_RELOAD_REG
+};
+
+/* For setting up register classes, loop through the 3 register classes mapping
+   into real registers, and skip the ANY class, which is just an OR of the
+   bits.  */
+#define FIRST_RELOAD_REG_CLASS	RELOAD_REG_GPR
+#define LAST_RELOAD_REG_CLASS	RELOAD_REG_VMX
+
+/* Map reload register type to a register in the register class.  */
+struct reload_reg_map_type {
+  const char *name;			/* Register class name.  */
+  int reg;				/* Register in the register class.  */
+};
+
+extern const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG];
+
+/* Mask bits for each register class, indexed per mode.  Historically the
+   compiler has been more restrictive which types can do PRE_MODIFY instead of
+   PRE_INC and PRE_DEC, so keep track of sepaate bits for these two.  */
+typedef unsigned char addr_mask_type;
+
+#define RELOAD_REG_VALID	0x0001	/* Mode valid in register..  */
+#define RELOAD_REG_MULTIPLE	0x0002	/* Mode takes multiple registers.  */
+#define RELOAD_REG_INDEXED	0x0004	/* Reg+reg addressing.  */
+#define RELOAD_REG_OFFSET	0x0008	/* Reg+offset addressing. */
+#define RELOAD_REG_PRE_INCDEC	0x0010	/* PRE_INC/PRE_DEC valid.  */
+#define RELOAD_REG_PRE_MODIFY	0x0020	/* PRE_MODIFY valid.  */
+#define RELOAD_REG_AND_M16	0x0040	/* AND -16 addressing.  */
+#define RELOAD_REG_QUAD_OFFSET	0x0080	/* quad offset is limited.  */
+
+/* Register type masks based on the type, of valid addressing modes.  */
+struct rs6000_reg_addr {
+  enum insn_code reload_load;		/* INSN to reload for loading. */
+  enum insn_code reload_store;		/* INSN to reload for storing.  */
+  enum insn_code reload_fpr_gpr;	/* INSN to move from FPR to GPR.  */
+  enum insn_code reload_gpr_vsx;	/* INSN to move from GPR to VSX.  */
+  enum insn_code reload_vsx_gpr;	/* INSN to move from VSX to GPR.  */
+  addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
+  bool scalar_in_vmx_p;			/* Scalar value can go in VMX.  */
+};
+
+extern struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
+
+/* Helper function to say whether a mode supports PRE_INC or PRE_DEC.  */
+static inline bool
+mode_supports_pre_incdec_p (machine_mode mode)
+{
+  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC)
+	  != 0);
+}
+
+/* Helper function to say whether a mode supports PRE_MODIFY.  */
+static inline bool
+mode_supports_pre_modify_p (machine_mode mode)
+{
+  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_MODIFY)
+	  != 0);
+}
+
+/* Return true if we have D-form addressing in altivec registers.  */
+static inline bool
+mode_supports_vmx_dform (machine_mode mode)
+{
+  return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0);
+}
+
+/* Return true if we have D-form addressing in VSX registers.  This addressing
+   is more limited than normal d-form addressing in that the offset must be
+   aligned on a 16-byte boundary.  */
+static inline bool
+mode_supports_dq_form (machine_mode mode)
+{
+  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET)
+	  != 0);
+}
+
+/* Helper function to return whether a MODE can do prefixed loads/stores.
+   VOIDmode is used when we are loading the pc-relative address into a base
+   register, but we are not using it as part of a memory operation.  As modes
+   add support for prefixed memory, they will be added here.  */
+
+static inline bool
+mode_supports_prefixed_address_p (machine_mode mode)
+{
+  return mode == VOIDmode;
+}
+
+\f
 /* Structure used to define the rs6000 stack */
 typedef struct rs6000_stack {
   int reload_completed;		/* stack info won't change from here on */
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 273617)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -155,8 +155,7 @@ bool rs6000_returns_struct = false;
 #endif
 
 /* Value is TRUE if register/mode pair is acceptable.  */
-static bool rs6000_hard_regno_mode_ok_p
-  [NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
+bool rs6000_hard_regno_mode_ok_p[NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
 
 /* Maximum number of registers needed for a given register class and mode.  */
 unsigned char rs6000_class_max_nregs[NUM_MACHINE_MODES][LIM_REG_CLASSES];
@@ -295,122 +294,22 @@ bool cpu_builtin_p = false;
    don't link in rs6000-c.c, so we can't call it directly.  */
 void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT, HOST_WIDE_INT);
 
-/* Simplfy register classes into simpler classifications.  We assume
-   GPR_REG_TYPE - FPR_REG_TYPE are ordered so that we can use a simple range
-   check for standard register classes (gpr/floating/altivec/vsx) and
-   floating/vector classes (float/altivec/vsx).  */
-
-enum rs6000_reg_type {
-  NO_REG_TYPE,
-  PSEUDO_REG_TYPE,
-  GPR_REG_TYPE,
-  VSX_REG_TYPE,
-  ALTIVEC_REG_TYPE,
-  FPR_REG_TYPE,
-  SPR_REG_TYPE,
-  CR_REG_TYPE
-};
-
 /* Map register class to register type.  */
-static enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
-
-/* First/last register type for the 'normal' register types (i.e. general
-   purpose, floating point, altivec, and VSX registers).  */
-#define IS_STD_REG_TYPE(RTYPE) IN_RANGE(RTYPE, GPR_REG_TYPE, FPR_REG_TYPE)
-
-#define IS_FP_VECT_REG_TYPE(RTYPE) IN_RANGE(RTYPE, VSX_REG_TYPE, FPR_REG_TYPE)
-
+enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
 
 /* Register classes we care about in secondary reload or go if legitimate
    address.  We only need to worry about GPR, FPR, and Altivec registers here,
    along an ANY field that is the OR of the 3 register classes.  */
-
-enum rs6000_reload_reg_type {
-  RELOAD_REG_GPR,			/* General purpose registers.  */
-  RELOAD_REG_FPR,			/* Traditional floating point regs.  */
-  RELOAD_REG_VMX,			/* Altivec (VMX) registers.  */
-  RELOAD_REG_ANY,			/* OR of GPR, FPR, Altivec masks.  */
-  N_RELOAD_REG
-};
-
-/* For setting up register classes, loop through the 3 register classes mapping
-   into real registers, and skip the ANY class, which is just an OR of the
-   bits.  */
-#define FIRST_RELOAD_REG_CLASS	RELOAD_REG_GPR
-#define LAST_RELOAD_REG_CLASS	RELOAD_REG_VMX
-
-/* Map reload register type to a register in the register class.  */
-struct reload_reg_map_type {
-  const char *name;			/* Register class name.  */
-  int reg;				/* Register in the register class.  */
-};
-
-static const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG] = {
+const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG] = {
   { "Gpr",	FIRST_GPR_REGNO },	/* RELOAD_REG_GPR.  */
   { "Fpr",	FIRST_FPR_REGNO },	/* RELOAD_REG_FPR.  */
   { "VMX",	FIRST_ALTIVEC_REGNO },	/* RELOAD_REG_VMX.  */
   { "Any",	-1 },			/* RELOAD_REG_ANY.  */
 };
 
-/* Mask bits for each register class, indexed per mode.  Historically the
-   compiler has been more restrictive which types can do PRE_MODIFY instead of
-   PRE_INC and PRE_DEC, so keep track of sepaate bits for these two.  */
-typedef unsigned char addr_mask_type;
-
-#define RELOAD_REG_VALID	0x01	/* Mode valid in register..  */
-#define RELOAD_REG_MULTIPLE	0x02	/* Mode takes multiple registers.  */
-#define RELOAD_REG_INDEXED	0x04	/* Reg+reg addressing.  */
-#define RELOAD_REG_OFFSET	0x08	/* Reg+offset addressing. */
-#define RELOAD_REG_PRE_INCDEC	0x10	/* PRE_INC/PRE_DEC valid.  */
-#define RELOAD_REG_PRE_MODIFY	0x20	/* PRE_MODIFY valid.  */
-#define RELOAD_REG_AND_M16	0x40	/* AND -16 addressing.  */
-#define RELOAD_REG_QUAD_OFFSET	0x80	/* quad offset is limited.  */
-
-/* Register type masks based on the type, of valid addressing modes.  */
-struct rs6000_reg_addr {
-  enum insn_code reload_load;		/* INSN to reload for loading. */
-  enum insn_code reload_store;		/* INSN to reload for storing.  */
-  enum insn_code reload_fpr_gpr;	/* INSN to move from FPR to GPR.  */
-  enum insn_code reload_gpr_vsx;	/* INSN to move from GPR to VSX.  */
-  enum insn_code reload_vsx_gpr;	/* INSN to move from VSX to GPR.  */
-  addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
-  bool scalar_in_vmx_p;			/* Scalar value can go in VMX.  */
-};
-
-static struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
-
-/* Helper function to say whether a mode supports PRE_INC or PRE_DEC.  */
-static inline bool
-mode_supports_pre_incdec_p (machine_mode mode)
-{
-  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC)
-	  != 0);
-}
-
-/* Helper function to say whether a mode supports PRE_MODIFY.  */
-static inline bool
-mode_supports_pre_modify_p (machine_mode mode)
-{
-  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_MODIFY)
-	  != 0);
-}
-
-/* Return true if we have D-form addressing in altivec registers.  */
-static inline bool
-mode_supports_vmx_dform (machine_mode mode)
-{
-  return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0);
-}
-
-/* Return true if we have D-form addressing in VSX registers.  This addressing
-   is more limited than normal d-form addressing in that the offset must be
-   aligned on a 16-byte boundary.  */
-static inline bool
-mode_supports_dq_form (machine_mode mode)
-{
-  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET)
-	  != 0);
-}
+/* Various low level information needed for addressing formats, register
+   allocation, etc. indexed by mode.  */
+struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
 
 /* Given that there exists at least one variable that is set (produced)
    by OUT_INSN and read (consumed) by IN_INSN, return true iff
@@ -13587,17 +13486,6 @@ rs6000_pltseq_template (rtx *operands, i
 }
 #endif
 
-/* Helper function to return whether a MODE can do prefixed loads/stores.
-   VOIDmode is used when we are loading the pc-relative address into a base
-   register, but we are not using it as part of a memory operation.  As modes
-   add support for prefixed memory, they will be added here.  */
-
-static bool
-mode_supports_prefixed_address_p (machine_mode mode)
-{
-  return mode == VOIDmode;
-}
-
 /* Function to return true if ADDR is a valid prefixed memory address that uses
    mode MODE.  */
 

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[David Edelsohn]

On Sat, Jul 20, 2019 at 12:13 PM Michael Meissner
<meissner@linux.ibm.com> wrote:
>
> I will be iterating on patch #9 and sending out a replacement shortly.
>
> This is patch #10.  It moves the various data structures from rs6000.c to
> rs6000-internal.h.  In the future, it will allow us to move more things from
> rs6000.c to other files.  In particular, I plan on adding a rs6000-prefixed.c
> file that has the specific support for prefixed instructions.
>
> It would also allow various functions for handling adressing and register
> assignment, etc. that could also be moved to a separate file as well.
>
> This patch just moves the data structure definitions and helper functions
> (mode_supports_*) to rs6000-internal.h.  The static data structures are changed
> to external, and definitions of the function are put in rs6000.c.  I did add 00
> to the bit mask definitions for RELOAD_REG_* because I plan to add at least one
> more mask (for DS-form addresses) in the future, and it will the values lined
> up.
>
> I have done a bootstrap on a little endian power8 system running Linux and
> there were no regressions in the test suite.  Can I check this into the trunk?

This patch needs to add rs6000-internal.h to tm_file in gcc/config.gcc
for all of the powerpc/rs6000 targets.  It also may need tm_p_file and
tm_d_file definitions.

Thanks, David

>
> 2019-07-20  Michael Meissner  <meissner@linux.ibm.com>
>
>         * config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p):
>         Move various declarations relating to addressing and register
>         allocation to rs6000-internal.h from rs6000.c so that in the
>         future we can move things out of rs6000.c.  Make the static arrays
>         global, and define them in rs6000.c.
>         (enum rs6000_reg_type): Likewise.
>         (reg_class_to_reg_type): Likewise.
>         (IS_STD_REG_TYPE): Likewise.
>         (IS_FP_VECT_REG_TYPE): Likewise.
>         (enum rs6000_reload_reg_type): Likewise.
>         (FIRST_RELOAD_REG_CLASS): Likewise.
>         (LAST_RELOAD_REG_CLASS): Likewise.
>         (struct reload_reg_map_type): Likewise.
>         (reload_reg_map): Likewise.
>         (RELOAD_REG_* macros): Likewise.
>         (struct rs6000_reg_addr): Likewise.
>         (reg_addr): Likewise.
>         (mode_supports_pre_incdec_): Likewise.
>         (mode_supports_pre_modify_p): Likewise.
>         (mode_supports_vmx_dform): Likewise.
>         (mode_supports_dq_form): Likewise.
>         (mode_supports_prefixed_address_p): Likewise.
>         * config/rs6000/rs6000.c (rs6000_hard_regno_mode_ok_p): Likewise.
>         (enum rs6000_reg_type): Likewise.
>         (reg_class_to_reg_type): Likewise.
>         (IS_STD_REG_TYPE): Likewise.
>         (IS_FP_VECT_REG_TYPE): Likewise.
>         (enum rs6000_reload_reg_type): Likewise.
>         (FIRST_RELOAD_REG_CLASS): Likewise.
>         (LAST_RELOAD_REG_CLASS): Likewise.
>         (struct reload_reg_map_type): Likewise.
>         (reload_reg_map): Likewise.
>         (RELOAD_REG_* macros): Likewise.
>         (struct rs6000_reg_addr): Likewise.
>         (reg_addr): Likewise.
>         (mode_supports_pre_incdec_): Likewise.
>         (mode_supports_pre_modify_p): Likewise.
>         (mode_supports_vmx_dform): Likewise.
>         (mode_supports_dq_form): Likewise.
>         (mode_supports_prefixed_address_p): Likewise.
>
> Index: gcc/config/rs6000/rs6000-internal.h
> ===================================================================
> --- gcc/config/rs6000/rs6000-internal.h (revision 273617)
> +++ gcc/config/rs6000/rs6000-internal.h (working copy)
> @@ -22,6 +22,134 @@
>  #ifndef GCC_RS6000_INTERNAL_H
>  #define GCC_RS6000_INTERNAL_H
>
> +/* Value is TRUE if register/mode pair is acceptable.  */
> +extern bool rs6000_hard_regno_mode_ok_p
> +  [NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
> +
> +/* Simplfy register classes into simpler classifications.  We assume
> +   GPR_REG_TYPE - FPR_REG_TYPE are ordered so that we can use a simple range
> +   check for standard register classes (gpr/floating/altivec/vsx) and
> +   floating/vector classes (float/altivec/vsx).  */
> +
> +enum rs6000_reg_type {
> +  NO_REG_TYPE,
> +  PSEUDO_REG_TYPE,
> +  GPR_REG_TYPE,
> +  VSX_REG_TYPE,
> +  ALTIVEC_REG_TYPE,
> +  FPR_REG_TYPE,
> +  SPR_REG_TYPE,
> +  CR_REG_TYPE
> +};
> +
> +/* Map register class to register type.  */
> +extern enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
> +
> +/* First/last register type for the 'normal' register types (i.e. general
> +   purpose, floating point, altivec, and VSX registers).  */
> +#define IS_STD_REG_TYPE(RTYPE) IN_RANGE(RTYPE, GPR_REG_TYPE, FPR_REG_TYPE)
> +
> +#define IS_FP_VECT_REG_TYPE(RTYPE) IN_RANGE(RTYPE, VSX_REG_TYPE, FPR_REG_TYPE)
> +
> +
> +/* Register classes we care about in secondary reload or go if legitimate
> +   address.  We only need to worry about GPR, FPR, and Altivec registers here,
> +   along an ANY field that is the OR of the 3 register classes.  */
> +
> +enum rs6000_reload_reg_type {
> +  RELOAD_REG_GPR,                      /* General purpose registers.  */
> +  RELOAD_REG_FPR,                      /* Traditional floating point regs.  */
> +  RELOAD_REG_VMX,                      /* Altivec (VMX) registers.  */
> +  RELOAD_REG_ANY,                      /* OR of GPR, FPR, Altivec masks.  */
> +  N_RELOAD_REG
> +};
> +
> +/* For setting up register classes, loop through the 3 register classes mapping
> +   into real registers, and skip the ANY class, which is just an OR of the
> +   bits.  */
> +#define FIRST_RELOAD_REG_CLASS RELOAD_REG_GPR
> +#define LAST_RELOAD_REG_CLASS  RELOAD_REG_VMX
> +
> +/* Map reload register type to a register in the register class.  */
> +struct reload_reg_map_type {
> +  const char *name;                    /* Register class name.  */
> +  int reg;                             /* Register in the register class.  */
> +};
> +
> +extern const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG];
> +
> +/* Mask bits for each register class, indexed per mode.  Historically the
> +   compiler has been more restrictive which types can do PRE_MODIFY instead of
> +   PRE_INC and PRE_DEC, so keep track of sepaate bits for these two.  */
> +typedef unsigned char addr_mask_type;
> +
> +#define RELOAD_REG_VALID       0x0001  /* Mode valid in register..  */
> +#define RELOAD_REG_MULTIPLE    0x0002  /* Mode takes multiple registers.  */
> +#define RELOAD_REG_INDEXED     0x0004  /* Reg+reg addressing.  */
> +#define RELOAD_REG_OFFSET      0x0008  /* Reg+offset addressing. */
> +#define RELOAD_REG_PRE_INCDEC  0x0010  /* PRE_INC/PRE_DEC valid.  */
> +#define RELOAD_REG_PRE_MODIFY  0x0020  /* PRE_MODIFY valid.  */
> +#define RELOAD_REG_AND_M16     0x0040  /* AND -16 addressing.  */
> +#define RELOAD_REG_QUAD_OFFSET 0x0080  /* quad offset is limited.  */
> +
> +/* Register type masks based on the type, of valid addressing modes.  */
> +struct rs6000_reg_addr {
> +  enum insn_code reload_load;          /* INSN to reload for loading. */
> +  enum insn_code reload_store;         /* INSN to reload for storing.  */
> +  enum insn_code reload_fpr_gpr;       /* INSN to move from FPR to GPR.  */
> +  enum insn_code reload_gpr_vsx;       /* INSN to move from GPR to VSX.  */
> +  enum insn_code reload_vsx_gpr;       /* INSN to move from VSX to GPR.  */
> +  addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
> +  bool scalar_in_vmx_p;                        /* Scalar value can go in VMX.  */
> +};
> +
> +extern struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
> +
> +/* Helper function to say whether a mode supports PRE_INC or PRE_DEC.  */
> +static inline bool
> +mode_supports_pre_incdec_p (machine_mode mode)
> +{
> +  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC)
> +         != 0);
> +}
> +
> +/* Helper function to say whether a mode supports PRE_MODIFY.  */
> +static inline bool
> +mode_supports_pre_modify_p (machine_mode mode)
> +{
> +  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_MODIFY)
> +         != 0);
> +}
> +
> +/* Return true if we have D-form addressing in altivec registers.  */
> +static inline bool
> +mode_supports_vmx_dform (machine_mode mode)
> +{
> +  return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0);
> +}
> +
> +/* Return true if we have D-form addressing in VSX registers.  This addressing
> +   is more limited than normal d-form addressing in that the offset must be
> +   aligned on a 16-byte boundary.  */
> +static inline bool
> +mode_supports_dq_form (machine_mode mode)
> +{
> +  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET)
> +         != 0);
> +}
> +
> +/* Helper function to return whether a MODE can do prefixed loads/stores.
> +   VOIDmode is used when we are loading the pc-relative address into a base
> +   register, but we are not using it as part of a memory operation.  As modes
> +   add support for prefixed memory, they will be added here.  */
> +
> +static inline bool
> +mode_supports_prefixed_address_p (machine_mode mode)
> +{
> +  return mode == VOIDmode;
> +}
> +
> +
>  /* Structure used to define the rs6000 stack */
>  typedef struct rs6000_stack {
>    int reload_completed;                /* stack info won't change from here on */
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 273617)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -155,8 +155,7 @@ bool rs6000_returns_struct = false;
>  #endif
>
>  /* Value is TRUE if register/mode pair is acceptable.  */
> -static bool rs6000_hard_regno_mode_ok_p
> -  [NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
> +bool rs6000_hard_regno_mode_ok_p[NUM_MACHINE_MODES][FIRST_PSEUDO_REGISTER];
>
>  /* Maximum number of registers needed for a given register class and mode.  */
>  unsigned char rs6000_class_max_nregs[NUM_MACHINE_MODES][LIM_REG_CLASSES];
> @@ -295,122 +294,22 @@ bool cpu_builtin_p = false;
>     don't link in rs6000-c.c, so we can't call it directly.  */
>  void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT, HOST_WIDE_INT);
>
> -/* Simplfy register classes into simpler classifications.  We assume
> -   GPR_REG_TYPE - FPR_REG_TYPE are ordered so that we can use a simple range
> -   check for standard register classes (gpr/floating/altivec/vsx) and
> -   floating/vector classes (float/altivec/vsx).  */
> -
> -enum rs6000_reg_type {
> -  NO_REG_TYPE,
> -  PSEUDO_REG_TYPE,
> -  GPR_REG_TYPE,
> -  VSX_REG_TYPE,
> -  ALTIVEC_REG_TYPE,
> -  FPR_REG_TYPE,
> -  SPR_REG_TYPE,
> -  CR_REG_TYPE
> -};
> -
>  /* Map register class to register type.  */
> -static enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
> -
> -/* First/last register type for the 'normal' register types (i.e. general
> -   purpose, floating point, altivec, and VSX registers).  */
> -#define IS_STD_REG_TYPE(RTYPE) IN_RANGE(RTYPE, GPR_REG_TYPE, FPR_REG_TYPE)
> -
> -#define IS_FP_VECT_REG_TYPE(RTYPE) IN_RANGE(RTYPE, VSX_REG_TYPE, FPR_REG_TYPE)
> -
> +enum rs6000_reg_type reg_class_to_reg_type[N_REG_CLASSES];
>
>  /* Register classes we care about in secondary reload or go if legitimate
>     address.  We only need to worry about GPR, FPR, and Altivec registers here,
>     along an ANY field that is the OR of the 3 register classes.  */
> -
> -enum rs6000_reload_reg_type {
> -  RELOAD_REG_GPR,                      /* General purpose registers.  */
> -  RELOAD_REG_FPR,                      /* Traditional floating point regs.  */
> -  RELOAD_REG_VMX,                      /* Altivec (VMX) registers.  */
> -  RELOAD_REG_ANY,                      /* OR of GPR, FPR, Altivec masks.  */
> -  N_RELOAD_REG
> -};
> -
> -/* For setting up register classes, loop through the 3 register classes mapping
> -   into real registers, and skip the ANY class, which is just an OR of the
> -   bits.  */
> -#define FIRST_RELOAD_REG_CLASS RELOAD_REG_GPR
> -#define LAST_RELOAD_REG_CLASS  RELOAD_REG_VMX
> -
> -/* Map reload register type to a register in the register class.  */
> -struct reload_reg_map_type {
> -  const char *name;                    /* Register class name.  */
> -  int reg;                             /* Register in the register class.  */
> -};
> -
> -static const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG] = {
> +const struct reload_reg_map_type reload_reg_map[N_RELOAD_REG] = {
>    { "Gpr",     FIRST_GPR_REGNO },      /* RELOAD_REG_GPR.  */
>    { "Fpr",     FIRST_FPR_REGNO },      /* RELOAD_REG_FPR.  */
>    { "VMX",     FIRST_ALTIVEC_REGNO },  /* RELOAD_REG_VMX.  */
>    { "Any",     -1 },                   /* RELOAD_REG_ANY.  */
>  };
>
> -/* Mask bits for each register class, indexed per mode.  Historically the
> -   compiler has been more restrictive which types can do PRE_MODIFY instead of
> -   PRE_INC and PRE_DEC, so keep track of sepaate bits for these two.  */
> -typedef unsigned char addr_mask_type;
> -
> -#define RELOAD_REG_VALID       0x01    /* Mode valid in register..  */
> -#define RELOAD_REG_MULTIPLE    0x02    /* Mode takes multiple registers.  */
> -#define RELOAD_REG_INDEXED     0x04    /* Reg+reg addressing.  */
> -#define RELOAD_REG_OFFSET      0x08    /* Reg+offset addressing. */
> -#define RELOAD_REG_PRE_INCDEC  0x10    /* PRE_INC/PRE_DEC valid.  */
> -#define RELOAD_REG_PRE_MODIFY  0x20    /* PRE_MODIFY valid.  */
> -#define RELOAD_REG_AND_M16     0x40    /* AND -16 addressing.  */
> -#define RELOAD_REG_QUAD_OFFSET 0x80    /* quad offset is limited.  */
> -
> -/* Register type masks based on the type, of valid addressing modes.  */
> -struct rs6000_reg_addr {
> -  enum insn_code reload_load;          /* INSN to reload for loading. */
> -  enum insn_code reload_store;         /* INSN to reload for storing.  */
> -  enum insn_code reload_fpr_gpr;       /* INSN to move from FPR to GPR.  */
> -  enum insn_code reload_gpr_vsx;       /* INSN to move from GPR to VSX.  */
> -  enum insn_code reload_vsx_gpr;       /* INSN to move from VSX to GPR.  */
> -  addr_mask_type addr_mask[(int)N_RELOAD_REG]; /* Valid address masks.  */
> -  bool scalar_in_vmx_p;                        /* Scalar value can go in VMX.  */
> -};
> -
> -static struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
> -
> -/* Helper function to say whether a mode supports PRE_INC or PRE_DEC.  */
> -static inline bool
> -mode_supports_pre_incdec_p (machine_mode mode)
> -{
> -  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_INCDEC)
> -         != 0);
> -}
> -
> -/* Helper function to say whether a mode supports PRE_MODIFY.  */
> -static inline bool
> -mode_supports_pre_modify_p (machine_mode mode)
> -{
> -  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_PRE_MODIFY)
> -         != 0);
> -}
> -
> -/* Return true if we have D-form addressing in altivec registers.  */
> -static inline bool
> -mode_supports_vmx_dform (machine_mode mode)
> -{
> -  return ((reg_addr[mode].addr_mask[RELOAD_REG_VMX] & RELOAD_REG_OFFSET) != 0);
> -}
> -
> -/* Return true if we have D-form addressing in VSX registers.  This addressing
> -   is more limited than normal d-form addressing in that the offset must be
> -   aligned on a 16-byte boundary.  */
> -static inline bool
> -mode_supports_dq_form (machine_mode mode)
> -{
> -  return ((reg_addr[mode].addr_mask[RELOAD_REG_ANY] & RELOAD_REG_QUAD_OFFSET)
> -         != 0);
> -}
> +/* Various low level information needed for addressing formats, register
> +   allocation, etc. indexed by mode.  */
> +struct rs6000_reg_addr reg_addr[NUM_MACHINE_MODES];
>
>  /* Given that there exists at least one variable that is set (produced)
>     by OUT_INSN and read (consumed) by IN_INSN, return true iff
> @@ -13587,17 +13486,6 @@ rs6000_pltseq_template (rtx *operands, i
>  }
>  #endif
>
> -/* Helper function to return whether a MODE can do prefixed loads/stores.
> -   VOIDmode is used when we are loading the pc-relative address into a base
> -   register, but we are not using it as part of a memory operation.  As modes
> -   add support for prefixed memory, they will be added here.  */
> -
> -static bool
> -mode_supports_prefixed_address_p (machine_mode mode)
> -{
> -  return mode == VOIDmode;
> -}
> -
>  /* Function to return true if ADDR is a valid prefixed memory address that uses
>     mode MODE.  */
>
>
> --
> 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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Michael Meissner]

On Sat, Jul 20, 2019 at 12:28:34PM -0400, David Edelsohn wrote:
> This patch needs to add rs6000-internal.h to tm_file in gcc/config.gcc
> for all of the powerpc/rs6000 targets.  It also may need tm_p_file and
> tm_d_file definitions.

While I agree it would make things easier if the declarations are available to
everybody, why wasn't this an issue when rs6000-internal.h was created to allow
stuff to move out from rs6000.c to rs6000-logues.c?

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

On Mon, Jul 22, 2019 at 02:34:53PM -0400, Michael Meissner wrote:
> On Sat, Jul 20, 2019 at 12:28:34PM -0400, David Edelsohn wrote:
> > This patch needs to add rs6000-internal.h to tm_file in gcc/config.gcc
> > for all of the powerpc/rs6000 targets.  It also may need tm_p_file and
> > tm_d_file definitions.
> 
> While I agree it would make things easier if the declarations are available to
> everybody, why wasn't this an issue when rs6000-internal.h was created to allow
> stuff to move out from rs6000.c to rs6000-logues.c?

AIUI it is mostly needed to get dependencies right?


Segher

Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> I will be iterating on patch #9 and sending out a replacement shortly.
> 
> This is patch #10.  It moves the various data structures from rs6000.c to

I'll review this tomorrow, fwiw.

A general request: please don't send patches as replies to other emails.


Segher

Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Michael Meissner]

On Sun, Jul 21, 2019 at 12:41:51PM -0500, Segher Boessenkool wrote:
> On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> > I will be iterating on patch #9 and sending out a replacement shortly.
> > 
> > This is patch #10.  It moves the various data structures from rs6000.c to
> 
> I'll review this tomorrow, fwiw.
> 
> A general request: please don't send patches as replies to other emails.

In the past you had asked me to group patches under a single overview patch.  I
can go back to sending patches individually.

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

On Mon, Jul 22, 2019 at 02:36:00PM -0400, Michael Meissner wrote:
> On Sun, Jul 21, 2019 at 12:41:51PM -0500, Segher Boessenkool wrote:
> > On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> > > I will be iterating on patch #9 and sending out a replacement shortly.
> > > 
> > > This is patch #10.  It moves the various data structures from rs6000.c to
> > 
> > I'll review this tomorrow, fwiw.
> > 
> > A general request: please don't send patches as replies to other emails.
> 
> In the past you had asked me to group patches under a single overview patch.  I
> can go back to sending patches individually.

When you send a *series*, please send it as series; when you send
individual patches, please send them individually.  This is all about
how people will get it in their email, and how they will handle it.

This really *is* a series, just most patches do not exist yet, if I
understand correctly.  That is not ideal of course -- I cannot review
it as a series, because I don't have the later patches.  Sending the
patches as replies to weeks-old other patches is less than useful.


Segher

Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

Hi!

On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> 2019-07-20  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p):
> 	Move various declarations relating to addressing and register
> 	allocation to rs6000-internal.h from rs6000.c so that in the
> 	future we can move things out of rs6000.c.

Just say
  (rs6000_hard_regno_mode_ok_p): New declaration.
for the things that only had a definition before.

> 	Make the static arrays global,

That's not this entry.  Say that in the entries where it applies.

> 	and define them in rs6000.c.

Say that in the corresponding entry for rs6000.c .

> 	(enum rs6000_reg_type): Likewise.

This one always was a declaration.

(... ten gazillion "Likewise." ...)
Most of those are *not* the same thing.  Don't say "likewise" if not
the same comment applies.

If it is hard to write a proper changelog, your patch series probably
could use some restructuring.  Or sometimes the changelog you need just
is more work than you would prefer.

You don't necessarily have to keep the same order in the changelog as
in the patch, if that helps.  But roughly the same order helps review,
so please consider that too ;-)

> +/* Simplfy register classes into simpler classifications.  We assume

(Typo, not new, but still a typo :-) )

> +/* Register classes we care about in secondary reload or go if legitimate
> +   address.  We only need to worry about GPR, FPR, and Altivec registers here,
> +   along an ANY field that is the OR of the 3 register classes.  */

We haven't had GO_IF_LEGITIMATE_ADDRESS for ten years now, please don't
introduce new references to it ;-)

> +#define RELOAD_REG_VALID	0x0001	/* Mode valid in register..  */
> +#define RELOAD_REG_MULTIPLE	0x0002	/* Mode takes multiple registers.  */
> +#define RELOAD_REG_INDEXED	0x0004	/* Reg+reg addressing.  */
> +#define RELOAD_REG_OFFSET	0x0008	/* Reg+offset addressing. */
> +#define RELOAD_REG_PRE_INCDEC	0x0010	/* PRE_INC/PRE_DEC valid.  */
> +#define RELOAD_REG_PRE_MODIFY	0x0020	/* PRE_MODIFY valid.  */
> +#define RELOAD_REG_AND_M16	0x0040	/* AND -16 addressing.  */
> +#define RELOAD_REG_QUAD_OFFSET	0x0080	/* quad offset is limited.  */

Why all the extra zeroes?  If you introduce some 0x100 later, just leave
the 0x80 as 0x80 please, that is much more readable.


It's hard to tell whether the problem is factored sanely, or if this
creates a big mountain of spaghetti instead.  Can you show how this is
used later?

Normally, you send a whole series, and then perhaps many of the first
are preparatory only, but a reviewer can see where things are headed,
and *then* simple refactorings like this can make sense.  The way this
patch looks now you are just making a lot of data global.


Segher

Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Michael Meissner]

On Mon, Jul 22, 2019 at 12:42:13AM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> > 2019-07-20  Michael Meissner  <meissner@linux.ibm.com>
> > 
> > 	* config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p):
> > 	Move various declarations relating to addressing and register
> > 	allocation to rs6000-internal.h from rs6000.c so that in the
> > 	future we can move things out of rs6000.c.
> 
> Just say
>   (rs6000_hard_regno_mode_ok_p): New declaration.
> for the things that only had a definition before.
> 
> > 	Make the static arrays global,
> 
> That's not this entry.  Say that in the entries where it applies.
> 
> > 	and define them in rs6000.c.
> 
> Say that in the corresponding entry for rs6000.c .
> 
> > 	(enum rs6000_reg_type): Likewise.
> 
> This one always was a declaration.

This was a mechanical patch, just moving the swath of code from rs6000.c to
rs6000-internal.h.

> (... ten gazillion "Likewise." ...)
> Most of those are *not* the same thing.  Don't say "likewise" if not
> the same comment applies.
> 
> If it is hard to write a proper changelog, your patch series probably
> could use some restructuring.  Or sometimes the changelog you need just
> is more work than you would prefer.
> 
> You don't necessarily have to keep the same order in the changelog as
> in the patch, if that helps.  But roughly the same order helps review,
> so please consider that too ;-)

Well yes, while in general I would prefer to group things logically together
for ChangeLog, I don't tend to do it because as you say it makes it easier for
the review.

> We haven't had GO_IF_LEGITIMATE_ADDRESS for ten years now, please don't
> introduce new references to it ;-)

Yep.

> > +#define RELOAD_REG_VALID	0x0001	/* Mode valid in register..  */
> > +#define RELOAD_REG_MULTIPLE	0x0002	/* Mode takes multiple registers.  */
> > +#define RELOAD_REG_INDEXED	0x0004	/* Reg+reg addressing.  */
> > +#define RELOAD_REG_OFFSET	0x0008	/* Reg+offset addressing. */
> > +#define RELOAD_REG_PRE_INCDEC	0x0010	/* PRE_INC/PRE_DEC valid.  */
> > +#define RELOAD_REG_PRE_MODIFY	0x0020	/* PRE_MODIFY valid.  */
> > +#define RELOAD_REG_AND_M16	0x0040	/* AND -16 addressing.  */
> > +#define RELOAD_REG_QUAD_OFFSET	0x0080	/* quad offset is limited.  */
> 
> Why all the extra zeroes?  If you introduce some 0x100 later, just leave
> the 0x80 as 0x80 please, that is much more readable.

As I mentioned, I will be adding at least one new bit (RELOAD_REG_DS_OFFSET),
but I have potential patches that add a few more (those are still in flux), and
those would need the extra column.

I personally DO NOT find mask definitions like:

#define RELOAD_REG_VALID	0x01	/* Mode valid in register..  */
#define RELOAD_REG_MULTIPLE	0x02	/* Mode takes multiple registers.  */
#define RELOAD_REG_INDEXED	0x04	/* Reg+reg addressing.  */
#define RELOAD_REG_OFFSET	0x08	/* Reg+offset addressing. */
#define RELOAD_REG_PRE_INCDEC	0x10	/* PRE_INC/PRE_DEC valid.  */
#define RELOAD_REG_PRE_MODIFY	0x20	/* PRE_MODIFY valid.  */
#define RELOAD_REG_AND_M16	0x40	/* AND -16 addressing.  */
#define RELOAD_REG_QUAD_OFFSET	0x80	/* quad offset is limited.  */
#define RELOAD_REG_DS_OFFSET	0x100	/* bottom 2 bits must be zero.  */

to be readable.  So in change, I can go back to just:

#define RELOAD_REG_VALID	0x01	/* Mode valid in register..  */
#define RELOAD_REG_MULTIPLE	0x02	/* Mode takes multiple registers.  */
#define RELOAD_REG_INDEXED	0x04	/* Reg+reg addressing.  */
#define RELOAD_REG_OFFSET	0x08	/* Reg+offset addressing. */
#define RELOAD_REG_PRE_INCDEC	0x10	/* PRE_INC/PRE_DEC valid.  */
#define RELOAD_REG_PRE_MODIFY	0x20	/* PRE_MODIFY valid.  */
#define RELOAD_REG_AND_M16	0x40	/* AND -16 addressing.  */
#define RELOAD_REG_QUAD_OFFSET	0x80	/* quad offset is limited.  */

And in the next patch, change it all to:

#define RELOAD_REG_VALID	0x001	/* Mode valid in register..  */
#define RELOAD_REG_MULTIPLE	0x002	/* Mode takes multiple registers.  */
#define RELOAD_REG_INDEXED	0x004	/* Reg+reg addressing.  */
#define RELOAD_REG_OFFSET	0x008	/* Reg+offset addressing. */
#define RELOAD_REG_PRE_INCDEC	0x010	/* PRE_INC/PRE_DEC valid.  */
#define RELOAD_REG_PRE_MODIFY	0x020	/* PRE_MODIFY valid.  */
#define RELOAD_REG_AND_M16	0x040	/* AND -16 addressing.  */
#define RELOAD_REG_QUAD_OFFSET	0x080	/* quad offset is limited.  */
#define RELOAD_REG_DS_OFFSET    0x100	/* bottom 2 bits must be 0.  */

Some time later if I continue with the current code, that would be changed to:

#define RELOAD_REG_VALID	0x0001	/* Mode valid in register..  */
#define RELOAD_REG_MULTIPLE	0x0002	/* Mode takes multiple registers.  */
#define RELOAD_REG_INDEXED	0x0004	/* Reg+reg addressing.  */
#define RELOAD_REG_OFFSET	0x0008	/* Reg+offset addressing. */
#define RELOAD_REG_PRE_INCDEC	0x0010	/* PRE_INC/PRE_DEC valid.  */
#define RELOAD_REG_PRE_MODIFY	0x0020	/* PRE_MODIFY valid.  */
#define RELOAD_REG_AND_M16	0x0040	/* AND -16 addressing.  */
#define RELOAD_REG_QUAD_OFFSET	0x0080	/* DQ-form (bottom 4 bits 0).  */
#define RELOAD_REG_DS_OFFSET	0x0100	/* DS-form (bottom 2 bits 0).  */

/* The following are flags used in address decoding, but are not currently
   stored in the addr_mask field of reg_addr.  */
#define RELOAD_REG_SINGLE	0x0200	/* Address is a single register.  */
#define RELOAD_REG_IS_PREFIXED	0x0400	/* Address uses prefix encoding.  */
#define RELOAD_REG_NOT_PREFIXED	0x0800	/* Address does not use a prefix.  */
#define RELOAD_REG_PCREL_LOCAL	0x1000	/* Pc-relative to local symbol.  */
#define RELOAD_REG_PCREL_EXT	0x2000	/* Pc-relative to external symbol.  */
#define RELOAD_REG_LO_SUM	0x4000	/* Address uses LO_SUM (TOC, etc).  */

I was just trying to save a step since I was moving the definitions any way to
add the alignment.

> 
> It's hard to tell whether the problem is factored sanely, or if this
> creates a big mountain of spaghetti instead.  Can you show how this is
> used later?

The intention is to move bits to other files.  In particular, I want to create
a new rs6000-prefixed.c file that contains the bits specific to prefixed
addressing.  However, a lot of the bits need access to the reg_addr array.
When you have the reg_addr array, all of the mode_supports_* functions also
become useful in the other files.

In addition to reg_array, the code that determines if something is a
traditional instruction or a prefixed instruction needs the register type
classificiation (rs6000_reg_type).  The issue is in some instructions it
depends on whether the offset is an instruction using the DS encoding or D
encoding.  For example, loading floating point scalars to a traditional FPR
register uses the D encoding, but loading floating point scalars to an altivec
register uses the DS encoding.

However, longer term, it would be nice to move other things from rs6000.c to
other files, such as the functions that deal with p8 fusion, and the legitimate
address functions (which I still tend to mentally classify as GO_IF_LEGITIMATE
type functions, even as you mention, we no longer have GO_IF_LEGITIMATE*).

> Normally, you send a whole series, and then perhaps many of the first
> are preparatory only, but a reviewer can see where things are headed,
> and *then* simple refactorings like this can make sense.  The way this
> patch looks now you are just making a lot of data global.

This was intended to be just a mechanical patch to move things to the .h file.

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

On Mon, Jul 22, 2019 at 02:59:39PM -0400, Michael Meissner wrote:
> On Mon, Jul 22, 2019 at 12:42:13AM -0500, Segher Boessenkool wrote:
> > On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote:
> > > 2019-07-20  Michael Meissner  <meissner@linux.ibm.com>
> > > 
> > > 	* config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p):
> > > 	Move various declarations relating to addressing and register
> > > 	allocation to rs6000-internal.h from rs6000.c so that in the
> > > 	future we can move things out of rs6000.c.
> > 
> > Just say
> >   (rs6000_hard_regno_mode_ok_p): New declaration.
> > for the things that only had a definition before.
> > 
> > > 	Make the static arrays global,
> > 
> > That's not this entry.  Say that in the entries where it applies.
> > 
> > > 	and define them in rs6000.c.
> > 
> > Say that in the corresponding entry for rs6000.c .
> > 
> > > 	(enum rs6000_reg_type): Likewise.
> > 
> > This one always was a declaration.
> 
> This was a mechanical patch, just moving the swath of code from rs6000.c to
> rs6000-internal.h.

And yet, your changelog should be correct ;-)

> So in change, I can go back to just:
> 
> #define RELOAD_REG_VALID	0x01	/* Mode valid in register..  */
> #define RELOAD_REG_MULTIPLE	0x02	/* Mode takes multiple registers.  */
> #define RELOAD_REG_INDEXED	0x04	/* Reg+reg addressing.  */
> #define RELOAD_REG_OFFSET	0x08	/* Reg+offset addressing. */
> #define RELOAD_REG_PRE_INCDEC	0x10	/* PRE_INC/PRE_DEC valid.  */
> #define RELOAD_REG_PRE_MODIFY	0x20	/* PRE_MODIFY valid.  */
> #define RELOAD_REG_AND_M16	0x40	/* AND -16 addressing.  */
> #define RELOAD_REG_QUAD_OFFSET	0x80	/* quad offset is limited.  */

Yes.  Just keep it as is.  This is supposed to be a move, not a change.

> Some time later if I continue with the current code, that would be changed to:

Maybe.  We'll see then.

> I was just trying to save a step since I was moving the definitions any way to
> add the alignment.

A move should not introduce any unnecessary changes.  This only is much
more work for everyone (you: you need to write extra changelog for it.
Me: I see there are extra changes, and now I have to check everything
else, too).

> > It's hard to tell whether the problem is factored sanely, or if this
> > creates a big mountain of spaghetti instead.  Can you show how this is
> > used later?
> 
> The intention is to move bits to other files.  In particular, I want to create
> a new rs6000-prefixed.c file that contains the bits specific to prefixed
> addressing.

That isn't a good split.  All addressing-related stuff in one file might
make sense, just like the rs6000-call.c split did (which was worthwhile
because all the huge builtin stuff naturally fit there as well).  But
only one kind of addressing?  Nope, that does not sound good.

> However, longer term, it would be nice to move other things from rs6000.c to
> other files, such as the functions that deal with p8 fusion, and the legitimate
> address functions (which I still tend to mentally classify as GO_IF_LEGITIMATE
> type functions, even as you mention, we no longer have GO_IF_LEGITIMATE*).

But only where that makes sense.  Dividing things is only good if a)
the division has an obvious, fundamental meaning; b) the interface
between that part and the rest is thin; c) there is something to actually
*win* from dividing things.

> > Normally, you send a whole series, and then perhaps many of the first
> > are preparatory only, but a reviewer can see where things are headed,
> > and *then* simple refactorings like this can make sense.  The way this
> > patch looks now you are just making a lot of data global.
> 
> This was intended to be just a mechanical patch to move things to the .h file.

That still needs an explanation: why is this a good thing, why do you
want that change?  Sometimes that is obvious of course, but here it is
not.  It would be a lot more obvious if there was more context.


Segher

Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Michael Meissner]

On Mon, Jul 22, 2019 at 03:56:26PM -0500, Segher Boessenkool wrote:
> That still needs an explanation: why is this a good thing, why do you
> want that change?  Sometimes that is obvious of course, but here it is
> not.  It would be a lot more obvious if there was more context.

The trouble is to get that much context really relies on about several
additional patches to get to the functions in particular that should be split
out.

As I implement stuff, I find myself neeting/wanting to access the stuff in
reg_addr in other files (predicates.md, and the new file rs6000-prefix.c).

In patch #9 in particular, I added yet another data stucture that was parallel
to the information in reg_addr (originally called rs6000_offset_format, but
based on your comments is now rs6000_insn_form for instruction format).

Then I needed functions that given were a hard register and the move insn and
it determined what instruction format (D/DS/DQ) was used so that we can decide
whether the instruction is traditional or prefixed.

I originally wrote with a lot of code to do that mapping, but once I added the
DS_OFFSET mask and about 10 lines of code to setup DS_OFFSET, it became much simpler.

But for now, I think I will just not worry about making rs6000.c smaller, and
instead do the main work there rather than trying to split it out.  I was
hoping the split would be quick, and it doesn't seem to be.  Splitting it out
is somewhat of a side issue to me, and it looks like it is impeding getting the
rest of the patches submitted.

But it would be nice to have that information available to the other .c files
as well as the .md files.

-- 
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], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h

[Segher Boessenkool]

Hi Mike,

On Mon, Jul 22, 2019 at 06:37:35PM -0400, Michael Meissner wrote:
> On Mon, Jul 22, 2019 at 03:56:26PM -0500, Segher Boessenkool wrote:
> > That still needs an explanation: why is this a good thing, why do you
> > want that change?  Sometimes that is obvious of course, but here it is
> > not.  It would be a lot more obvious if there was more context.
> 
> The trouble is to get that much context really relies on about several
> additional patches to get to the functions in particular that should be split
> out.

In the normal workflow, you send a series of patches when it is ready.
And if a series is not ready, you do not send it.

You can also give context just in the emails, or send a work-in-progress
patch just as FYI (please mark it clearly as such, then).  But without
context, how can I see if what a patch does is correct and useful?

> As I implement stuff, I find myself neeting/wanting to access the stuff in
> reg_addr in other files (predicates.md, and the new file rs6000-prefix.c).

Then you first need to question if things are split up in a useful way
the way things are, and then how it could be done better.

> But it would be nice to have that information available to the other .c files
> as well as the .md files.

If many other modules need the data, then either:
a) The module boundaries are not set nicely; or
b) This is a central data structure, and should be treated as one.


Segher