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?
> 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, "] ");
> + 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.
> 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?
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?)
Thanks,
Segher
