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.

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
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@quasilyte
Copy link
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 cmd/internal/obj/x86: invalid encoding of VPERMQ with negative imm arg #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
Copy link
Contributor Author

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
Copy link
Contributor Author

CC @TocarIP just in case.

@TocarIP
Copy link
Contributor

TocarIP commented May 16, 2018

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

@mvdan
Copy link
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
Copy link

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

@quasilyte
Copy link
Contributor Author

@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
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
Copy link
Contributor Author

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 23, 2018
@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
Copy link
Contributor

What is left to do on this issue?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Dec 7, 2018
@quasilyte
Copy link
Contributor Author

I believe it's resolved.

@quasilyte
Copy link
Contributor Author

(Seems like it can be safely closed.)

@golang golang locked and limited conversation to collaborators Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants