Skip to content

[AMDGPU] Give some of the VI V_PK_* instructions a subtarget predicate. #70334

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
Oct 27, 2023

Conversation

kosarev
Copy link
Collaborator

@kosarev kosarev commented Oct 26, 2023

This resolves AsmParser ambiguity between them and similar instructions for other subtargets, e.g., V_PK_SUB_U16_vi being identical to V_PK_SUB_U16_gfx11 on GFX11.

Part of #69256.

This resolves AsmParser ambiguity between them and similar instructions
for other subtargets, e.g., V_PK_SUB_U16_vi being identical to
V_PK_SUB_U16_gfx11 on GFX11.

Part of <llvm#69256>.
@llvmbot
Copy link
Member

llvmbot commented Oct 26, 2023

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

This resolves AsmParser ambiguity between them and similar instructions for other subtargets, e.g., V_PK_SUB_U16_vi being identical to V_PK_SUB_U16_gfx11 on GFX11.

Part of <#69256>.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/VOP3PInstructions.td (+2-1)
diff --git a/llvm/lib/Target/AMDGPU/VOP3PInstructions.td b/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
index 05e68f46b32605d..9c746cda1a4b108 100644
--- a/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
+++ b/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
@@ -1105,6 +1105,7 @@ multiclass VOP3P_Real_SMFMAC<bits<7> op, string alias> {
   def : MnemonicAlias<alias, !cast<VOP3_Pseudo>(NAME#"_e64").Mnemonic>;
 }
 
+let SubtargetPredicate = isGFX8GFX9 in {
 defm V_PK_MAD_I16 : VOP3P_Real_vi <0x00>;
 defm V_PK_MUL_LO_U16 : VOP3P_Real_vi <0x01>;
 defm V_PK_ADD_I16 : VOP3P_Real_vi <0x02>;
@@ -1125,7 +1126,7 @@ defm V_PK_ADD_F16 : VOP3P_Real_vi <0x0f>;
 defm V_PK_MUL_F16 : VOP3P_Real_vi <0x10>;
 defm V_PK_MIN_F16 : VOP3P_Real_vi <0x11>;
 defm V_PK_MAX_F16 : VOP3P_Real_vi <0x12>;
-
+} // End SubtargetPredicate = isGFX8GFX9
 
 let SubtargetPredicate = HasMadMixInsts in {
 defm V_MAD_MIX_F32 : VOP3P_Real_vi <0x20>;

Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm assuming "part of" implies there's a second patch which adds tests

@kosarev
Copy link
Collaborator Author

kosarev commented Oct 26, 2023

lgtm assuming "part of" implies there's a second patch which adds tests

'Part of' is supposed to mean there will be more patches, but I don't know how to test this now, because reproducing the problem would require changing the order in which instructions are matched in AsmParser. Improving existing ambiguity checks could help with this and is also seen part of the ticket, but doing that first, despite being generally preferable, would significantly delay starting working on the support for structured initialiser syntax, which is now a work of a priority. The plan is therefore that we try to mitigate the immediate problem with added/changed operand classes causing failures on regression tests, then deliver the structured initialiser syntax, and get back to the problem with ambiguous instructions.

Copy link
Collaborator

@rampitec rampitec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have seen similar problems with other instructions, very tricky to detect.

@kosarev kosarev merged commit f0899ed into llvm:main Oct 27, 2023
@kosarev kosarev deleted the fix_asmparser_ambiguities branch October 27, 2023 10:14
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.

4 participants