-
Notifications
You must be signed in to change notification settings - Fork 18k
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
Comments
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). |
CC @TocarIP just in case. |
FWIW negative immediate helps with porting code from gnu as, which accepts both. |
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. |
Change https://golang.org/cl/113615 mentions this issue: |
@mvdan, I've CCed you to the CL just in case. OfftopIt's actually me who should say "apologies", since I've missed that in initial CL during review. Better yet, no one gives apologies and we just get things fixed. :) |
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>
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. |
What is left to do on this issue? |
I believe it's resolved. |
(Seems like it can be safely closed.) |
During fix of #24378 new issues were introduced due to shared ytab list between
VPERMPD
andVPERMQ
: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)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.
The text was updated successfully, but these errors were encountered: