Skip to content

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

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

arsenm
Copy link
Contributor

@arsenm arsenm commented Jul 11, 2024

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

arsenm commented Jul 11, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @arsenm and the rest of your teammates on Graphite Graphite

@llvmbot
Copy link
Member

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/98516.diff

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp (+3-3)
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) {

@arsenm arsenm marked this pull request as ready for review July 11, 2024 18:52
Copy link
Contributor

@Sisyph Sisyph left a 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.

@arsenm
Copy link
Contributor Author

arsenm commented Jul 11, 2024

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?

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

Or set IsSingle on them? If we don't need a bit in TSFlags, it's better to not use it imo.

It is set, they don't end up in the VOP3InfoTable though because they are isCodeGenOnly

@Sisyph
Copy link
Contributor

Sisyph commented Jul 12, 2024

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?

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

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?

@arsenm
Copy link
Contributor Author

arsenm commented Jul 12, 2024

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

@Sisyph
Copy link
Contributor

Sisyph commented Jul 12, 2024

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.

@arsenm arsenm merged commit b3f5c72 into main Jul 12, 2024
11 checks passed
@arsenm arsenm deleted the users/arsenm/amdgpu-assume-true-vop-is-single-helpers branch July 12, 2024 17:12
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants