rs6000: PR target/95347 Correctly identify stfs if prefixed

Message ID 20200529213109.44674-1-acsawdey@linux.ibm.com
State New
Headers show
Series
  • rs6000: PR target/95347 Correctly identify stfs if prefixed
Related show

Commit Message

Kees Cook via Gcc-patches May 29, 2020, 9:31 p.m.
Because reg_to_non_prefixed() only looks at the register being used, it
doesn't get the right answer for stfs, which leads to us not seeing
that it has a PCREL symbol ref.  This patch works around this by
introducing a helper function that inspects the insn to see if it is in
fact a stfs. Then if we use NON_PREFIXED_DEFAULT, address_to_insn_form()
can see that it has the PCREL symbol ref.

OK for trunk if regstrap on ppc64le passes?

Thanks,
   Aaron

2020-05-29  Aaron Sawdey  <acsawdey@linux.ibm.com>

	PR target/95347
	* config/rs6000/rs6000.c (prefixed_store_p): Add special case
	for stfs.
	(is_stfs_insn): New helper function.
---
 gcc/config/rs6000/rs6000.c | 60 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Segher Boessenkool May 29, 2020, 10:02 p.m. | #1
Hi!

Re: [PATCH] rs6000: PR target/95347 Correctly identify stfs if prefixed
Please put the PR id at the end of the subject (it is the least
important information).  You can also shorten it to "PR95347" -- total
subject length ideally is maybe 50 chars, so something like
"rtl-optimization" would be extremely long, for no good reason.

On Fri, May 29, 2020 at 04:31:09PM -0500, Aaron Sawdey via Gcc-patches wrote:
> Because reg_to_non_prefixed() only looks at the register being used, it

> doesn't get the right answer for stfs, which leads to us not seeing

> that it has a PCREL symbol ref.  This patch works around this by

> introducing a helper function that inspects the insn to see if it is in

> fact a stfs. Then if we use NON_PREFIXED_DEFAULT, address_to_insn_form()

> can see that it has the PCREL symbol ref.


> +/* Helper function to see if we're potentially looking at stfs that

> +   could be pstfs.  */


"That could be pstfs" is only confusing here, I think?  It has nothing
to do with this function itself.

"Return true if INSN is a "movsi_from_sf" to memory"?

> +static bool

> +is_stfs_insn (rtx_insn *insn)

> +{

> +  rtx pattern=PATTERN (insn);


Spaces on both sides of binary operators (like "=").

> +  if (GET_CODE (pattern) != PARALLEL)

> +    return false;

> +

> +  /* This should be a parallel with exactly one set and one clobber.  */


You could simplify this: it has to be a parallel of exactly two things,
the first a SET, the second a CLOBBER?

> +  int i;

> +  rtx set=NULL, clobber=NULL;

> +  for (i = 0; i < XVECLEN (pattern, 0); i++)


  rtx set = NULL;
  rtx clobber = NULL;
  for (int i = 0; i < XVECLEN (pattern, 0); i++)

(Declarations with initialiser go on a line by their own; "i" doesn't
need declaring before the loop).

> +  /* All we care is that the destination of the SET is a mem:SI,

> +     the source should be an UNSPEC_SI_FROM_SF, and the clobber

> +     should be a scratch:V4SF.  */

> +

> +  rtx dest = XEXP (set, 0);


rtx dest = SET_DEST (set);

> +  rtx src = XEXP (set, 1);


rtx src = SET_SRC (set);

> +  rtx scratch = XEXP (clobber, 0);


rtx scratch = SET_DEST (clobber);

> @@ -25119,8 +25171,14 @@ prefixed_store_p (rtx_insn *insn)

>      return false;

>  

>    machine_mode mem_mode = GET_MODE (mem);

> +  rtx addr = XEXP (mem, 0);

>    enum non_prefixed_form non_prefixed = reg_to_non_prefixed (reg, mem_mode);

> -  return address_is_prefixed (XEXP (mem, 0), mem_mode, non_prefixed);

> +  /* Need to make sure we aren't looking at a stfs which doesn't

> +     looking like the other things that we are looking for.  */


s/looking/look/ I guess?

> +  if (non_prefixed == NON_PREFIXED_X && is_stfs_insn (insn))

> +    return address_is_prefixed (addr, mem_mode, NON_PREFIXED_DEFAULT);

> +  else

> +    return address_is_prefixed (addr, mem_mode, non_prefixed);


Rest looks fine :-)  Okay for trunk with the nits fixed and the
suggestions looked at.  Also okay for 10, if wanted there?

Thanks!


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 8435bc15d72..d58fceeeea4 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -24980,6 +24980,58 @@  address_to_insn_form (rtx addr,
   return INSN_FORM_BAD;
 }
 
+/* Helper function to see if we're potentially looking at stfs that
+   could be pstfs.  */
+
+static bool
+is_stfs_insn (rtx_insn *insn)
+{
+  rtx pattern=PATTERN (insn);
+  if (GET_CODE (pattern) != PARALLEL)
+    return false;
+
+  /* This should be a parallel with exactly one set and one clobber.  */
+  int i;
+  rtx set=NULL, clobber=NULL;
+  for (i = 0; i < XVECLEN (pattern, 0); i++)
+    {
+      rtx elt = XVECEXP (pattern, 0, i);
+      if (GET_CODE (elt) == SET)
+	{
+	  if (set)
+	    return false;
+	  set = elt;
+	}
+      else if (GET_CODE (elt) == CLOBBER)
+	{
+	  if (clobber)
+	    return false;
+	  clobber = elt;
+	}
+      else
+	return false;
+    }
+
+  /* All we care is that the destination of the SET is a mem:SI,
+     the source should be an UNSPEC_SI_FROM_SF, and the clobber
+     should be a scratch:V4SF.  */
+
+  rtx dest = XEXP (set, 0);
+  rtx src = XEXP (set, 1);
+  rtx scratch = XEXP (clobber, 0);
+
+  if (GET_CODE (src) != UNSPEC || XINT (src, 1) != UNSPEC_SI_FROM_SF)
+    return false;
+
+  if (GET_CODE (dest) != MEM || GET_MODE (dest) != SImode)
+    return false;
+
+  if (GET_CODE (scratch) != SCRATCH || GET_MODE (scratch) != V4SFmode)
+    return false;
+
+  return true;
+}
+
 /* Helper function to take a REG and a MODE and turn it into the non-prefixed
    instruction format (D/DS/DQ) used for offset memory.  */
 
@@ -25119,8 +25171,14 @@  prefixed_store_p (rtx_insn *insn)
     return false;
 
   machine_mode mem_mode = GET_MODE (mem);
+  rtx addr = XEXP (mem, 0);
   enum non_prefixed_form non_prefixed = reg_to_non_prefixed (reg, mem_mode);
-  return address_is_prefixed (XEXP (mem, 0), mem_mode, non_prefixed);
+  /* Need to make sure we aren't looking at a stfs which doesn't
+     looking like the other things that we are looking for.  */
+  if (non_prefixed == NON_PREFIXED_X && is_stfs_insn (insn))
+    return address_is_prefixed (addr, mem_mode, NON_PREFIXED_DEFAULT);
+  else
+    return address_is_prefixed (addr, mem_mode, non_prefixed);
 }
 
 /* Whether a load immediate or add instruction is a prefixed instruction.  This