[PATCH] rs6000: PR target/95347 Correctly identify stfs if prefixed

[Segher Boessenkool]

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