On Wed, Feb 17, 2021 at 06:09:39PM -0600, Segher Boessenkool wrote:
> Hi!
>
> On Wed, Feb 17, 2021 at 12:17:30PM -0500, Michael Meissner wrote:
> > I noticed that the power10 xxspltiw, xxspltidp, and xxsplti32dx
> > instructions are not flagged as prefixed instructions, which means the
> > instruction length is not set to 12 bytes. This patch sets these
> > instructions to be prefixed. It also ensures that a leading 'p' is not
> > emitted before the instruction.
> >
> > I checked this patch by doing a bootstrap build/check on a little endian
> > power8
> > server system. There were no regressions.
>
> 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, The main consequence of not setting the prefixed attribute is
that the insn length is wrong. Normal functions probably won't even notice,
but it is certainly possible that some function is large enough and it uses
xxsplatiw (and friends) and the assembler would complain that conditional jumps
are too far.
At the moment given it is only generated as a built-in function, I doubt people
would run into it. As we add support in GCC 12 to use these instructions to
load up constants into the vector registers, it is more likely that people will
run into issues if the length is wrong.
> > In addition, I debugged cc1, and I
> > put a breakpoint on the get_attr_length function and I verified the insns
> > now
> > have length 12.
>
> You can just use -dp; the generated assembler output has lines like
> pla 3,.LC0@pcrel # 6 [c=4 l=12] *pcrel_local_addr
> (c is cost, l is length).
>
> fprintf (asm_out_file, "[c=%d",
> insn_cost (debug_insn, optimize_insn_for_speed_p ()));
> if (HAVE_ATTR_length)
> fprintf (asm_out_file, " l=%d",
> get_attr_length (debug_insn));
> fprintf (asm_out_file, "] ");
Sure, but it is just as simple to verify it with a debugger.
> > + 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.
> > 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.
>
> There are four kinds of insns now:
>
> 1) Never prefixed.
> 2) Always prefixed, like xxspltiw but many more in the future.
> 3) Sometimes prefixed, and they get a "p" mnemonic prefix then. Those
> are the majority currently, since we mostly have load/store insns that
> are prefixed currently, and those usually have a non-prefixed form we
> handled already.
> 4) Sometimes prefixed, and they get a "pm" prefix then. We don't handle
> those yet (those are the MMA GER insns).
>
> The "prefixed" attribute should just mean if the instruction ended up as
> some prefixed form (8 bytes).
>
> 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.
--
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
