On Fri, Mar 12, 2021 at 03:34:09PM +0100, Uros Bizjak wrote:
> > (define_insn "*avx2_pmaddwd"
> > - [(set (match_operand:V8SI 0 "register_operand" "=x,v")
> > + [(set (match_operand:V8SI 0 "register_operand" "=Yw")
>
> I'm not sure contraction like this is correct. The prolbem is with vex
> vs. evex prefix, used in length calculation. When XMM16+ is used in
> the insn, and evex prefix is used, the unpatched version returns vex
> for alternative 0 due to (x,x,x) and evex for alternative 1, since one
> of registers satisfies only "v".
>
> Patched version now always emits vex, which is wrong for XMM16+ registers.
That is true, but we have many thousands of those cases, where we just
use vex or maybe_vex or maybe_evex prefix with v or Yv or Yw or YW etc.
Adding extra alternatives for that would mean changing pretty much all of
sse.md.
I think it is much easier to imply length_evex when prefix is vex/maybe_vex when
any of the operands is EXT_REX_SSE_REG_P, subreg thereof,
MASK_REG_P, or subreg thereof.
The default definition of the "length" attribute has:
(ior (eq_attr "prefix" "evex")
(and (ior (eq_attr "prefix" "maybe_evex")
(eq_attr "prefix" "maybe_vex"))
(match_test "TARGET_AVX512F")))
(plus (attr "length_evex")
(plus (attr "length_immediate")
(plus (attr "modrm")
(attr "length_address"))))
(ior (eq_attr "prefix" "vex")
(and (ior (eq_attr "prefix" "maybe_vex")
(eq_attr "prefix" "maybe_evex"))
(match_test "TARGET_AVX")))
(plus (attr "length_vex")
(plus (attr "length_immediate")
(plus (attr "modrm")
(attr "length_address"))))]
That is just extremely rough guess, assuming all insns with
evex/maybe_evex/maybe_vex prefix will be EVEX encoded when TARGET_AVX512F
is clearly wrong, that is only true for instructions that don't have
a VEX counterpart (e.g. if they have mnemonics that is EVEX only), otherwise
it depends on whether either the operands will be 64-byte (we can perhaps
use for that the mode attribute at least by default) or whether any of the
operands is %[xy]mm16+ or %k*.
So (but I think this must be GCC 12 material) I'd say we should throw away
maybe_evex prefix altogether (replace with maybe_vex or vex),
use evex for the cases where it has EVEX only mnemonics and otherwise
call some function to look at the operands and mode attribute.
The maybe_vex decision on TARGET_AVX vs. !TARGET_AVX is on the other side
correct, we have all those %v etc. to make sure we emit VEX/EVEX encoded
instructions with -mavx rather than the old SSE* encodings.
I think some years ago I've used scripts and -dP etc. to verify the length
computations for vex, for evex it is known for years that it is inaccurate
unfortunately.
Maybe it is conservatively correct? length_evex is hardcoded to 5,
while length_vex is 3 + 1 or 2 + 1.
In that case, perhaps I should for now change the vex on that particular
instruction to maybe_vex. But I see still several hundreds of instructions
with vex prefix attribute and v etc. in constraints.
So, if we want a conservatively correct fix now, I'd say we should treat
even "vex" in the "length_evex" condition, so (untested):
2021-03-12 Jakub Jelinek <ja...@redhat.com>
* config/i386/i386.md (length): For TARGET_AVX512F treat
also vex prefix conservatively as length_evex.
--- gcc/config/i386/i386.md.jj 2021-03-05 21:51:33.675350463 +0100
+++ gcc/config/i386/i386.md 2021-03-12 16:24:49.302919436 +0100
@@ -686,6 +686,7 @@
(attr "length_address")))
(ior (eq_attr "prefix" "evex")
(and (ior (eq_attr "prefix" "maybe_evex")
+ (eq_attr "prefix" "vex")
(eq_attr "prefix" "maybe_vex"))
(match_test "TARGET_AVX512F")))
(plus (attr "length_evex")
and if we eventually do something more accurate, perhaps vex could
stand for do the operand >= %xmm16 or %k? check (VEX can't ever encode
those), but perhaps don't do the mode attribute check?
Jakub
