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.

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

cmd/internal/obj/x86: VPERMPD invalid encoding and too permissive argument types #25420

Closed
Quasilyte opened this Issue May 16, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@Quasilyte
Contributor

Quasilyte commented May 16, 2018

During fix of #24378 new issues were introduced due to shared ytab list between VPERMPD and VPERMQ:

  1. Like in #25418, encoding is invalid due to missing optab bytes. (already fixed)
  2. Initial issue was with "signed only" immediate arguments. Now it accepts both. Could only accept unsigned. (Read rationale below.)

If someone used negative args after CL100475, they would get invalid encoding. Their code is broken and they would've noticed that and, hopefully, reported an issue. Seems no one did that, maybe no one relied on negative arguments in their hand-coded asm or their code is not executed anymore. We could reduce "special permissive instructions" baggage by omitting this one at least until someone notices and reports that negative arguments stopped to compile (at this point, they should be happy, because if it was compiled before, it would behave in unpredictable way).

Because VEX and EVEX optabs and ytabs may soon be fully auto-generated, it's quite unfortunate to add more and more exceptions that make instruction args more permissive (Russ Cox would use "sloppy" word here, I think).

I also have a plan of leveraging immediate arg checks into deeper part of encoder to make it possible to report better errors than "invalid instruction" for immediate arg overflow or invalid signedness, so regardless of what we decide, the minimal solution is just fixing the invalid encoding problem before auto-generated tables are updated and then addressing the second issue separately, in a more clean way.

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented May 16, 2018

The second part of the issue is important because AVX512-enabled asm which features fully auto-generated [E]VEX optabs does not includes only unsigned immediate argument type (since there were no tests for signed consts and no issues that asked for more permissive rules).

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented May 16, 2018

CC @TocarIP just in case.

@TocarIP

This comment has been minimized.

Contributor

TocarIP commented May 16, 2018

FWIW negative immediate helps with porting code from gnu as, which accepts both.

@mvdan

This comment has been minimized.

Member

mvdan commented May 16, 2018

My knowledge of assembly and of the assembler are limited, so apologies for breaking stuff. Of course, feel free to revert my commit - but it would be nice if the original issue remained fixed.

@gopherbot

This comment has been minimized.

gopherbot commented May 17, 2018

Change https://golang.org/cl/113615 mentions this issue: cmd/internal/obj/x86: fix VPERMQ and VPERMPD ytab

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented May 17, 2018

@mvdan, I've CCed you to the CL just in case.

Offtop

It's actually me who should say "apologies", since I've missed that in initial CL during review.
The deal with y-tables and op-tables is not that straightforward, nor pretty..

Better yet, no one gives apologies and we just get things fixed. :)

gopherbot pushed a commit that referenced this issue May 17, 2018

cmd/internal/obj/x86: fix VPERMQ and VPERMPD ytab
Fixes invalid encoding of VPERMQ and VPERMPD that use
negative immediate argument.

Fixes #25418
Updates #25420

Change-Id: Idd8180c4c632a76b76f3a68efd5f930d94431994
Reviewed-on: https://go-review.googlesource.com/113615
Run-TryBot: Iskander Sharipov <iskander.sharipov@intel.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented May 17, 2018

For keeping things in sync in both tip and avx512-enabled branches, I've made VPERMPD accept negative arguments as in the CL above.

If there are objections, they will can affect this decision.

@andybons andybons added this to the Go1.11 milestone May 23, 2018

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 23, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Dec 7, 2018

What is left to do on this issue?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 7, 2018

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented Dec 7, 2018

I believe it's resolved.

@Quasilyte

This comment has been minimized.

Contributor

Quasilyte commented Dec 8, 2018

(Seems like it can be safely closed.)

@Quasilyte Quasilyte closed this Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment