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

[AMDGPU] Fix subtarget predicates for some V_MAD, V_FMA and V_DOT instructions. #71194

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

kosarev
Copy link
Collaborator

@kosarev kosarev commented Nov 3, 2023

Resolves AsmParser ambiguities, e.g., between V_FMA_MIXLO_F16_vi and V_FMA_MIXLO_F16_gfx11.

Part of #69256.

@llvmbot
Copy link
Member

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-backend-amdgpu

Author: Ivan Kosarev (kosarev)

Changes

Resolves AsmParser ambiguities, e.g., between V_FMA_MIXLO_F16_vi and V_FMA_MIXLO_F16_gfx11.

Part of <#69256>.


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

1 Files Affected:

  • (modified) llvm/lib/Target/AMDGPU/VOP3PInstructions.td (+11-18)
diff --git a/llvm/lib/Target/AMDGPU/VOP3PInstructions.td b/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
index 24d7550e2dec496..a6dfac3bc26dc49 100644
--- a/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
+++ b/llvm/lib/Target/AMDGPU/VOP3PInstructions.td
@@ -373,36 +373,31 @@ class SDot2Pat<Instruction Inst> : GCNPat <
 }
 
 let IsDOT = 1 in {
-let SubtargetPredicate = HasDot2Insts in {
-
+let OtherPredicates = [HasDot2Insts] in {
 defm V_DOT2_I32_I16 : VOP3PInst<"v_dot2_i32_i16",
   VOP3P_Profile<VOP_I32_V2I16_V2I16_I32>, int_amdgcn_sdot2, 1>;
 defm V_DOT2_U32_U16 : VOP3PInst<"v_dot2_u32_u16",
   VOP3P_Profile<VOP_I32_V2I16_V2I16_I32>, int_amdgcn_udot2, 1>;
+} // End OtherPredicates = [HasDot2Insts]
 
-} // End SubtargetPredicate = HasDot2Insts
-
-let SubtargetPredicate = HasDot10Insts in
+let OtherPredicates = [HasDot10Insts] in
 defm V_DOT2_F32_F16 : VOP3PInst<"v_dot2_f32_f16",
   VOP3P_Profile<VOP_F32_V2F16_V2F16_F32, VOP3_REGULAR, /*HasDPP*/ 1>,
   AMDGPUfdot2, 1/*ExplicitClamp*/>;
 
-let SubtargetPredicate = HasDot7Insts in {
+let OtherPredicates = [HasDot7Insts] in {
 defm V_DOT4_U32_U8  : VOP3PInst<"v_dot4_u32_u8",
   VOP3P_Profile<VOP_I32_I32_I32_I32, VOP3_PACKED>, int_amdgcn_udot4, 1>;
 defm V_DOT8_U32_U4  : VOP3PInst<"v_dot8_u32_u4",
   VOP3P_Profile<VOP_I32_I32_I32_I32, VOP3_PACKED>, int_amdgcn_udot8, 1>;
+} // End OtherPredicates = [HasDot7Insts]
 
-} // End SubtargetPredicate = HasDot7Insts
-
-let SubtargetPredicate = HasDot1Insts in {
-
+let OtherPredicates = [HasDot1Insts] in {
 defm V_DOT4_I32_I8  : VOP3PInst<"v_dot4_i32_i8",
   VOP3P_Profile<VOP_I32_I32_I32_I32, VOP3_PACKED>, int_amdgcn_sdot4, 1>;
 defm V_DOT8_I32_I4  : VOP3PInst<"v_dot8_i32_i4",
   VOP3P_Profile<VOP_I32_I32_I32_I32, VOP3_PACKED>, int_amdgcn_sdot8, 1>;
-
-} // End SubtargetPredicate = HasDot1Insts
+} // End OtherPredicates = [HasDot1Insts]
 
 def DOT2_BF16_Profile
   : VOP3P_Profile<VOP_F32_V2I16_V2I16_F32, VOP3_REGULAR, /*HasDPP*/ 1> {
@@ -1128,16 +1123,15 @@ 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 {
+let OtherPredicates = [HasMadMixInsts] in {
 defm V_MAD_MIX_F32 : VOP3P_Real_vi <0x20>;
 defm V_MAD_MIXLO_F16 : VOP3P_Real_vi <0x21>;
 defm V_MAD_MIXHI_F16 : VOP3P_Real_vi <0x22>;
 }
 
-let SubtargetPredicate = HasFmaMixInsts in {
-let DecoderNamespace = "GFX9_DL" in {
+let OtherPredicates = [HasFmaMixInsts],
+    DecoderNamespace = "GFX9_DL" in {
 // The mad_mix instructions were renamed and their behaviors changed,
 // but the opcode stayed the same so we need to put these in a
 // different DecoderNamespace to avoid the ambiguity.
@@ -1145,8 +1139,6 @@ defm V_FMA_MIX_F32 : VOP3P_Real_vi <0x20>;
 defm V_FMA_MIXLO_F16 : VOP3P_Real_vi <0x21>;
 defm V_FMA_MIXHI_F16 : VOP3P_Real_vi <0x22>;
 }
-}
-
 
 defm V_DOT2_I32_I16 : VOP3P_Real_vi <0x26>;
 defm V_DOT2_U32_U16 : VOP3P_Real_vi <0x27>;
@@ -1157,6 +1149,7 @@ defm V_DOT8_U32_U4  : VOP3P_Real_vi <0x2b>;
 
 defm V_DOT4_I32_I8  : VOP3P_Real_vi <0x28>;
 defm V_DOT8_I32_I4  : VOP3P_Real_vi <0x2a>;
+} // End SubtargetPredicate = isGFX8GFX9
 
 let OtherPredicates = [HasMAIInsts] in {
 

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

LGTM.

How do you find these problems? Once they are all fixed can you add some kind of assertion somewhere to stop them reoccuring?

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.

LGTM

@kosarev
Copy link
Collaborator Author

kosarev commented Nov 3, 2023

How do you find these problems? Once they are all fixed can you add some kind of assertion somewhere to stop them reoccuring?

I use the ambiguity checks in AsmMatcherEmitter, but it also takes making local changes manually teaching it about which of our predicates are mutually exclusive. I do hope to be able to make it catching that sort of problems without manual help at some point later (as part of #69256), but it currently feels like a long way to go.

…tructions.

Resolves AsmParser ambiguities, e.g., between V_FMA_MIXLO_F16_vi and
V_FMA_MIXLO_F16_gfx11.

Part of <llvm#69256>.
@kosarev kosarev force-pushed the asmparser_fix_ambiguities branch from 12e95f4 to 5112bd0 Compare November 7, 2023 12:53
@kosarev
Copy link
Collaborator Author

kosarev commented Nov 7, 2023

Fixed pattern predicates as well to pass codegen tests (must have missed the failures). Please take another look.

@kosarev kosarev requested review from jayfoad and Sisyph November 7, 2023 12:57
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -360,7 +360,7 @@ class UDot2Pat<Instruction Inst> : GCNPat <
(and i32:$src1, (i32 65535)))
),
(Inst (i32 8), $src0, (i32 8), $src1, (i32 8), $src2, (i1 0))> {
let SubtargetPredicate = !cast<VOP_Pseudo>(Inst).SubtargetPredicate;
let Predicates = !cast<VOP_Pseudo>(Inst).Predicates;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow up maybe change the template argument to VOP_Pseudo Inst to avoid the cast?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kosarev kosarev merged commit 429098c into llvm:main Nov 7, 2023
@kosarev kosarev deleted the asmparser_fix_ambiguities branch November 7, 2023 13:33
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