x/tools/go/analysis: asmdecl false positives on AVX2 instructions

[Yawning]

go vet raises invalid VPBROADCASTD of mask+24(FP); uint32 is 4-byte value for VPBROADCASTD mask+24(FP), Y0.

Skimming the asmdecl source, this is because argument size is determined by the final character of the opcode (plus some special cases), but when AVX2 instructions were added, the decision was made to use the actual (ie: Intel's) name of each instruction. I suspect there are other opcodes that also trigger false positives in this way.

[zpavlinovic]

Could we get a reproducer code? What would also help is to have the opening comment follow the default issue formatter.

[Yawning]

Could we get a reproducer code? What would also help is to have the opening comment follow the default issue formatter.

I narrowed down and root caused the issue because presumably using a giant wall of assembly isn't all that fun, but ok.

What version of Go are you using (go version)?

go version go1.16.6 linux/amd64
go version go1.17rc2 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

This is irrelevant, it's a logic bug in asmdecl.

What did you do?

Ran go vet on an assembly file that contains a memory-sourced VPBROADCASTD (VEX.256.66.0F38.W0 58 /r VPBROADCASTD ymm1, xmm2/m32), where there's size information available for the m32 due to it being an argument (mask+24(FP), mask is a uint32).

https://github.com/oasisprotocol/curve25519-voi/blob/4da56de5ed176dc827391508a16115b250ca91ad/curve/edwards_vector_amd64.s#L201

What did you expect to see?

No asmdecl vet errors regarding VPBROADCASTD. There's other things that asmdecl complains about in the file that are unrelated (the code generator I'm using so I need to copy-paste · less, is incorrect).

What did you see instead?

./edwards_vector_amd64.s:201:1: [amd64] vecConditionalSelect_AVX2: invalid VPBROADCASTD of mask+24(FP); uint32 is 4-byte value

What's wrong

https://github.com/golang/tools/blob/758a1a1db799319be7c0ac33d911e0e5107e48d3/go/analysis/passes/asmdecl/asmdecl.go#L717 is the generic catch-all for opcodes that end with D, following the plan 9 convention. AVX2 opcodes even in Go's assembler use Intel's naming, so the D means "dword".

[mmcloughlin]

Maybe related: #39220.

[aclements]

FWIW, I would be fine with adding this as a special case to asmdecl, either to recognize the AVX2 instructions, or to recognize the Yn registers.

We have long-term plans (#17544) to replace asmdecl, which is just a bunch of ad hoc regeps, with integration directly into the assembler so it can use the real parser, but that isn't high on the priority list right now.

[Yawning]

FWIW, I would be fine with adding this as a special case to asmdecl, either to recognize the AVX2 instructions, or to recognize the Yn registers.

For the specific VPBROADCASTD case that I am hitting, I believe this needs the former? asmdecl is complaining that I am passing it a GP register that contains the address of a uint32 (xmm2/m32). It is fine with the YMM register I am broadcasting into.

  // ie: Something like this should be "fine", but it costs an extra instruction. 
  VMOVD mask+(24)FP, X0
  VPBROADCASTD X0, Y0