Re: [PATCH], Patch #2 of 10, Add RTL prefixed attribute

[Segher Boessenkool]

Hi Mike,

Some comments on this patch:

On Wed, Aug 14, 2019 at 05:59:13PM -0400, Michael Meissner wrote:
> Due to some of the existing load and store insns not using the traditional
> operands[0] and operands[1], the functions that test whether an insn is
> prefixed only use the insn and not the operands directly.

Both the "update" and the "indexed" attributes have no problem with
this: the insns that have the problem set the attribute value directly.
This is mainly all the various update insns, but there are a bunch more,
and they all need different settings for their attributes.

>   * config/rs6000/rs6000.c (rs6000_emit_move): Add support for
>   loading up pc-relatve addresses.

(typo btw)

> +void
> +rs6000_final_prescan_insn (rtx_insn *insn)
> +{
> +  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> +  return;
> +}

Don't say "return;" at the end of a function please.

> +void
> +rs6000_asm_output_opcode (FILE *stream, const char *)
> +{
> +  if (next_insn_prefixed_p)
> +{
> +  next_insn_prefixed_p = false;
> +  fprintf (stream, "p");
> +}

You don't need to clear the flag here; the next call to
rs6000_final_prescan_insn will.

> +#define FINAL_PRESCAN_INSN(INSN, OPERANDS, NOPERANDS)
> \
> +do   \
> +  {  \
> +if (TARGET_PREFIXED_ADDR)
> \
> +  rs6000_final_prescan_insn (INSN);  
> \
> +  }  \
> +while (0)

Either have the function only do what it needs to for prefixed, and call
it something with prefixed in the name, or put the TARGET_PREFIXED_ADDR
test in the function itself.

> +;; Load up a pc-relative address.  ASM_OUTPUT_OPCODE will emit the initial 
> "p".
> +(define_insn "*pcrel_addr"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r")
> + (match_operand:DI 1 "pcrel_address"))]
> +  "TARGET_PCREL"
> +  "la %0,%a1"
> +  [(set_attr "prefixed" "yes")])

(use P for addresses please)

> +;; Load up a pc-relative address to an external symbol.  If the symbol and 
> the
> +;; program are both defined in the main program, the linker will optimize 
> this
> +;; to a PADDI.  Otherwise, it will create a GOT address that is relocated by
> +;; the dynamic linker and loaded up.
> +(define_insn "*pcrel_ext_addr"
> +  [(set (match_operand:DI 0 "gpc_reg_operand" "=b*r")
> + (match_operand:DI 1 "pcrel_external_address"))]
> +  "TARGET_PCREL"
> +  "ld %0,%a1"
> +  [(set_attr "prefixed" "yes")])

pld does an indirection more than pla does, but this is not clear at all
from the RTL, from the predicate names.  All this is *before* the linker
has done its thing, so pcrel_external_address is really some GOT memory,
so it should have that in its name.


Segher

[PATCH], Patch #2 of 10, Add RTL prefixed attribute

[Michael Meissner]

This patch adds the RTL attribute "prefixed" that says this particular
instruction is a prefixed instruction.

The target hooks FINAL_SCAN_INSN and ASM_OUTPUT_OPCODE are defined.  If the
insn is prefixed, ASM_OUTPUT_OPCODE will emit a leading 'p' before the
instruction is emitted.  For example, a load word and zero extend instruction
would have the output template:

lwz%U1%X1 %0,%1

If the insn is prefixed, ASM_OUTPUT_OPCODE will emit the leading 'p' and the
assembler would see something like:

plwz 3,foo@pcrel

The RTL length attribute looks a the RTL prefixed attribute to set the default
length to either 4 or 12.

In order to simplify setting the length for complex insns that aren't yet
split, I have added two new RTL attributes ("non_prefixed_length" and
"prefixed_length") that the length attribute uses.  Normally these values would
be 4 and 12 bytes, unless this is overwritten by the insn attributes.

In previous versions of the patch, I had a maybe_prefixed attribute that was
used to say this instruction might be prefixed, and to check whether the
instruction was prefixed externally.  Now, I use the type RTL attribute, and I
only look if the type is one of the load types, one of the store types, or one
of the integer and add types.

Due to some of the existing load and store insns not using the traditional
operands[0] and operands[1], the functions that test whether an insn is
prefixed only use the insn and not the operands directly.

Most of the new  code is in a new file (rs6000-prefixed.c).

2019-08-14   Michael Meissner  

* config/rs6000/rs6000-prefixed.c: New file.
* config/rs6000/rs6000-protos.h (rs6000_final_prescan_insn):
Update calling signature.
(prefixed_load_p): New function.
(prefixed_store_p): New function.
(prefixed_paddi_p): New function.
* config/rs6000/rs6000.c (rs6000_emit_move): Add support for
loading up pc-relatve addresses.
* config/rs6000/rs6000.h (FINAL_SCAN_INSN): New target hook.
(ASM_OUTPUT_OPCODE): New target hook.
* config/rs6000/rs6000.md (prefixed attribute): New attribute.
(prefixed_length attribute): New attribute.
(non_prefixed_length attribute): New attribute.
(length attribute): Calculate length in terms of the prefixed,
prefixed_length, and non_prefixed_length attributes.
(pcrel_addr): New insn for pc-relative support.
(pcrel_ext_addr): New insn for pc-relative support.
* config/rs6000/t-rs6000 (rs6000-prefixed.o): Add build rule.
* config.gcc (powerpc*-*-*): Add rs6000-prefixed.c.
(rs6000*-*-*): Add rs6000-prefixed.c.

Index: gcc/config/rs6000/rs6000-prefixed.c
===
--- gcc/config/rs6000/rs6000-prefixed.c (revision 0)
+++ gcc/config/rs6000/rs6000-prefixed.c (working copy)
@@ -0,0 +1,188 @@
+/* Subroutines used to support prefixed addressing on the PowerPC.
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of GCC.
+
+   GCC is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published
+   by the Free Software Foundation; either version 3, or (at your
+   option) any later version.
+
+   GCC is distributed in the hope that it will be useful, but WITHOUT
+   ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+   or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+   License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GCC; see the file COPYING3.  If not see
+   .  */
+
+#define IN_TARGET_CODE 1
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "backend.h"
+#include "rtl.h"
+#include "tree.h"
+#include "memmodel.h"
+#include "df.h"
+#include "tm_p.h"
+#include "ira.h"
+#include "print-tree.h"
+#include "varasm.h"
+#include "explow.h"
+#include "expr.h"
+#include "output.h"
+#include "tree-pass.h"
+#include "rtx-vector-builder.h"
+#include "print-rtl.h"
+#include "insn-attr.h"
+#include "insn-config.h"
+#include "recog.h"
+#include "tm-constrs.h"
+
+/* Whether the next instruction needs a 'p' prefix issued before the
+   instruction is printed out.  */
+static bool next_insn_prefixed_p;
+
+/* Define FINAL_PRESCAN_INSN if some processing needs to be done before
+   outputting the assembler code.  On the PowerPC, we remember if the current
+   insn is a prefixed insn where we need to emit a 'p' before the insn.  */
+void
+rs6000_final_prescan_insn (rtx_insn *insn)
+{
+  next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
+  return;
+}
+
+/* Define ASM_OUTPUT_OPCODE to do anything special before emitting an opcode.
+   We use it to emit a 'p' for prefixed insns that is set in
+   FINAL_PRESCAN_INSN.  We also use it for PCREL_OPT to emit the relocation
+   that ties the load of the GOT