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
