On Thu, Feb 18, 2021 at 10:44:14PM -0500, Michael Meissner wrote:
> On Wed, Feb 17, 2021 at 06:09:39PM -0600, Segher Boessenkool wrote:
> > Why test a p10 patch on a p8?
>
> Well it is just a code gen patch, so I can test it on any system, even an
> x86_64 with a cross compiler. Given that right now xxsplatiw is only created
> via a built-in,
That in itself should be fixed btw (don't use unspecs).
> > > + Some prefixed instructions (xxspltiw, xxspltidp, xxsplti32dp, etc.)
> > > do not
> > > + have a leading 'p'. Setting the prefix attribute to special does not
> > > the 'p'
> > > + prefix. */
> >
> > (grammar)
> >
> > "special" is the *normal* case. *Most* prefixed insns will be like
> > this. They aren't right now, but they will be.
> >
> > It sounds like you should make a new attribute, "always_prefixed" or
> > something, that then the code that sets "prefixed" can use.
>
> Yes agreed.
Or better yet, "maybe_prefixed", which you set when something else will
make the decision. Mmostly the default value of the prefixed attribute
will then do that. See "maybe_var_shift" / "var_shift" for example.
> > > void
> > > rs6000_final_prescan_insn (rtx_insn *insn, rtx [], int)
> > > {
> > > - next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO);
> > > + next_insn_prefixed_p = (get_attr_prefixed (insn) != PREFIXED_NO
> > > + && get_attr_prefixed (insn) != PREFIXED_SPECIAL);
> > > return;
> > > }
> >
> > So you set next_insn_prefixed_p when exactly? The original code was
> > correct, as far as I can see?
>
> rs6000_final_prescan_insn is called in the FINAL_PRESCAN_INSN target hook, and
> it is the only place we can set the flag for ASM_OUTPUT_OPCODE to print the
> initial 'p'. As you know, during the development of prefixed support, I went
> through various different methods of generating the prefixed instruction
> (i.e. pld instead of ld). The current method works for normal load, store and
> add instructions that have a normal form and a prefixed form. But it doesn't
> work for other instructions that we need to start dealing with.
Ah, the flag doesn't mean that the next insn is prefixed. It means we
want to prefix the mnemonic with a "p", instead. So some name change is
in order (as a separate, trivial, patch at the start of the series) --
this helps bisection if needed later, but it also helps reviewing
enormously).
> > So, for insns like xxspltiw you should just set "prefixed" to "yes",
> > because that makes sense, is not confusing. The code that prefixes "p"
> > to the mnemonic should change, instead. It can look at some new
> > attribute, but it could also just use
> > prefixed_load_p (insn) || prefixed_store_p (insn)
> > || prefixed_paddi_p (insn)
> > or similar (perhaps make a helper function for that then?)
>
> Yes. Pat Haugen will be taking over the PR.
Thanks,
Segher
