-
Notifications
You must be signed in to change notification settings - Fork 14.3k
AMDGPU: Assume true in getVOPNIsSingle helpers #98516
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
AMDGPU: Assume true in getVOPNIsSingle helpers #98516
Conversation
If we have something we don't know what it is, we should conservatively avoid printing an additional suffix. For isCodeGenOnly pseudoinstructions, no encoded instruction is added to the tables this is queried, and the null case would assume true. This happens to fix the case I ran into, but this isn't a wholistic fix. These really should be encoded directly in the TSFlags of the MCInstrDesc, which would allow encoding pseudos to work correctly.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@llvm/pr-subscribers-backend-amdgpu Author: Matt Arsenault (arsenm) ChangesIf we have something we don't know what it is, we should conservatively This happens to fix the case I ran into, but this isn't a wholistic fix. Full diff: https://github.com/llvm/llvm-project/pull/98516.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
index 9886235121d25..1b3cc4a83bea3 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
@@ -496,17 +496,17 @@ bool getSMEMIsBuffer(unsigned Opc) {
bool getVOP1IsSingle(unsigned Opc) {
const VOPInfo *Info = getVOP1OpcodeHelper(Opc);
- return Info ? Info->IsSingle : false;
+ return Info ? Info->IsSingle : true;
}
bool getVOP2IsSingle(unsigned Opc) {
const VOPInfo *Info = getVOP2OpcodeHelper(Opc);
- return Info ? Info->IsSingle : false;
+ return Info ? Info->IsSingle : true;
}
bool getVOP3IsSingle(unsigned Opc) {
const VOPInfo *Info = getVOP3OpcodeHelper(Opc);
- return Info ? Info->IsSingle : false;
+ return Info ? Info->IsSingle : true;
}
bool isVOPC64DPP(unsigned Opc) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be relying on the default behavior. So wouldn't it make sense to mark your instructions of interest as isCodeGenOnly = 0, if they are actually getting printed? Or set IsSingle on them?
If we don't need a bit in TSFlags, it's better to not use it imo.
They are all printed and real instructions, except for the purposes of disassembly. They have ambiguous decodings we need to deal with, so the encoded aliases need to be isCodeGenOnly
It is set, they don't end up in the VOP3InfoTable though because they are isCodeGenOnly |
This seems like a contradiction to me, and is asking for a different kind of fix. But if you want to fix it this way, can you add a test, so someone else doesn't come along and try to switch it back later? |
The situation is a mess. We have isCodeGenOnly and isAsmParserOnly, so you can construct printable but not disassembled pseudo instructions. I can't test this directly but it will break all the codegen and disassembler tests in a future commit |
Based on that it sounds like things will eventually reach a reasonable state. And this patch is NFC in tree, so it does not hurt. Ok with me. |
If we have something we don't know what it is, we should conservatively avoid printing an additional suffix. For isCodeGenOnly pseudoinstructions, no encoded instruction is added to the tables this is queried, and the null case would assume true. This happens to fix the case I ran into, but this isn't a wholistic fix. These really should be encoded directly in the TSFlags of the MCInstrDesc, which would allow encoding pseudos to work correctly.
If we have something we don't know what it is, we should conservatively
avoid printing an additional suffix. For isCodeGenOnly pseudoinstructions,
no encoded instruction is added to the tables this is queried, and the null
case would assume true.
This happens to fix the case I ran into, but this isn't a wholistic fix.
These really should be encoded directly in the TSFlags of the MCInstrDesc,
which would allow encoding pseudos to work correctly.