Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/asm: wrong AVX512 instructions are miscompiled #57952

Closed
WojciechMula opened this issue Jan 22, 2023 · 5 comments
Closed

cmd/asm: wrong AVX512 instructions are miscompiled #57952

WojciechMula opened this issue Jan 22, 2023 · 5 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Milestone
Unplanned

Comments

@WojciechMula
Copy link

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

$ go version
go version go1.19.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I compile this simple assembly procedure:

TEXT ·procedure(SB), NOSPLIT|NOFRAME, $0-8
    VMOVDQA32.Z Z0, Z1
    RET

Because I used .Z suffix, it means the instruction needs a mask register. That is missing in this case -- it should be something like VMOVDQA32.Z Z0, K1, Z1.

What did you expect to see?

Assembler message pointing out a syntax error.

What did you see instead?

The procedure is compiled into the following instructions (the output from GNU objdump):

0000000000457c80 <main.procedure.abi0>:
  457c80:	62 f1 7d c8 7f c1    	vmovdqa32 %zmm0,%zmm1{z}
  457c86:	c3                   	retq   
  457c87:	cc                   	int3   
  457c88:	cc                   	int3   
  457c89:	cc                   	int3   
  457c8a:	cc                   	int3   
  457c8b:	cc                   	int3   
  457c8c:	cc                   	int3   
  457c8d:	cc                   	int3   
  457c8e:	cc                   	int3   
  457c8f:	cc                   	int3

When the program is run, we get a CPU exception:

SIGILL: illegal instruction
PC=0x457c80 m=0 sigcode=2
instruction bytes: 0x62 0xf1 0x7d 0xc8 0x7f 0xc1 0xc3 0xcc 0xcc 0xcc 0xcc 0xcc 0xcc 0xcc 0xcc 0xcc
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 22, 2023
@randall77
Copy link
Contributor

So should we produce an error in this case then? i.e., if there is a .Z suffix but no mask register specified?

@WojciechMula
Copy link
Author

So should we produce an error in this case then? i.e., if there is a .Z suffix but no mask register specified?

Yes, it's an error. An instruction without .Z suffix is valid in two forms, for instance:

VMOVDQA32 Z0, Z1 // a regular move without masking
VMOVDQA32 Z0, K1, Z1 // merge-move using mask K1

But in the case of zeroing variant VMOVDQA32.Z a mask register is mandatory - this particular instruction means: copy elements for which the bit in the mask is 1, write zeros when the bit is 0. Without a mask reg the instruction has no sense.

@randall77
Copy link
Contributor

@quasilyte

@randall77 randall77 added this to the Unplanned milestone Jan 27, 2023
@gopherbot
Copy link

Change https://go.dev/cl/463229 mentions this issue: cmd/asm: reject avx512 .Z instructions without a mask register

@wojciech-sneller
Copy link

Thank you for such a quick fix! Great.

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 12, 2023
@randall77 @johanbrandhorst
cmd/asm: reject avx512 .Z instructions without a mask register
64f3b28 
Zeroing requires a non-K0 mask register be specified.
(gcc enforces this when assembling.)

The non-K0 restriction is already handled by the Yknot0 restriction.
But if the mask register is missing altogether, we misassemble the
instruction.

Fixes golang#57952

Not sure if this is really worth mentioning in the release notes,
but just in case I'll mark it.
RELNOTE=yes

Change-Id: I8f05d3155503f1f16d1b5ab9d67686fe5b64dfea
Reviewed-on: https://go-review.googlesource.com/c/go/+/463229
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Илья Токарь <tocarip@gmail.com>
Reviewed-by: Iskander Sharipov <quasilyte@gmail.com>
@golang golang locked and limited conversation to collaborators Jan 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@WojciechMula @randall77 @gopherbot @wojciech-sneller