[AMDGPU] Move renamedInGFX9 from TableGen to SIInstrInfo helper function/macro to free up a bit slot #82787

CRobeck · 2024-02-23T16:31:32Z

Follow on to #81525 and #81901 in the series of consolidating bits in TSFlags.

Remove renamedInGFX9 from SIInstrFormats.td and move to helper function/macro in SIInstrInfo. renamedInGFX9 points to V_{add, sub, subrev, addc, subb, subbrev}_ U32 and V_{div_fixup_F16, fma_F16, interp_p2_F16, mad_F16, mad_U16, mad_I16}.

llvmbot · 2024-02-23T16:32:01Z

@llvm/pr-subscribers-backend-amdgpu

Author: Corbin Robeck (CRobeck)

Changes

Follow on to #81525 and #81901 in the series of consolidating bits in TSFlags.

Remove renamedInGFX9 from SIInstrFormats.td and add a helper function to SIInstrInfo. renamedInGFX9 points to V_{add, sub, subrev, addc, subb, subbrev}U32 and V{div_fixup_F16, fma_F16, interp_p2_F16, mad_F16, mad_U16, mad_I16}.

It's pretty ugly to just have a check against the giant list of renamed instructions but the list is set and won't grow so might be the best option.

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

5 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIInstrFormats.td (+3-5)
(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+70-2)
(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.h (+2)
(modified) llvm/lib/Target/AMDGPU/VOP2Instructions.td (+29-49)
(modified) llvm/lib/Target/AMDGPU/VOP3Instructions.td (+20-22)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrFormats.td b/llvm/lib/Target/AMDGPU/SIInstrFormats.td
index 327eb89efcb88c..e6674a18000b69 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrFormats.td
+++ b/llvm/lib/Target/AMDGPU/SIInstrFormats.td
@@ -84,10 +84,6 @@ class InstSI <dag outs, dag ins, string asm = "",
   // Is it possible for this instruction to be atomic?
   field bit maybeAtomic = 1;
 
-  // This bit indicates that this is a VI instruction which is renamed
-  // in GFX9. Required for correct mapping from pseudo to MC.
-  field bit renamedInGFX9 = 0;
-
   // This bit indicates that this has a floating point result type, so
   // the clamp modifier has floating point semantics.
   field bit FPClamp = 0;
@@ -214,7 +210,9 @@ class InstSI <dag outs, dag ins, string asm = "",
   let TSFlags{42} = VOP3_OPSEL;
 
   let TSFlags{43} = maybeAtomic;
-  let TSFlags{44} = renamedInGFX9;
+
+  // Reserved, must be 0.
+  let TSFlags{44} = 0;
 
   let TSFlags{45} = FPClamp;
   let TSFlags{46} = IntClamp;
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 31ced9d41e15e2..ff19a7a6c60a31 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -9115,14 +9115,82 @@ bool SIInstrInfo::isAsmOnlyOpcode(int MCOp) const {
   }
 }
 
+bool SIInstrInfo::isRenamedInGFX9(int Opcode) const {
+  switch (Opcode) {
+  case AMDGPU::V_ADDC_U32_dpp:
+  case AMDGPU::V_ADDC_U32_e32:
+  case AMDGPU::V_ADDC_U32_e64:
+  case AMDGPU::V_ADDC_U32_e64_dpp:
+  case AMDGPU::V_ADDC_U32_sdwa:
+  //
+  case AMDGPU::V_ADD_CO_U32_dpp:
+  case AMDGPU::V_ADD_CO_U32_e32:
+  case AMDGPU::V_ADD_CO_U32_e64:
+  case AMDGPU::V_ADD_CO_U32_e64_dpp:
+  case AMDGPU::V_ADD_CO_U32_sdwa:
+  //
+  case AMDGPU::V_ADD_U32_dpp:
+  case AMDGPU::V_ADD_U32_e32:
+  case AMDGPU::V_ADD_U32_e64:
+  case AMDGPU::V_ADD_U32_e64_dpp:
+  case AMDGPU::V_ADD_U32_sdwa:
+  //
+  case AMDGPU::V_DIV_FIXUP_F16_gfx9_e64:
+  case AMDGPU::V_FMA_F16_gfx9_e64:
+  case AMDGPU::V_INTERP_P2_F16:
+  case AMDGPU::V_MAD_F16_e64:
+  case AMDGPU::V_MAD_U16_e64:
+  case AMDGPU::V_MAD_I16_e64:
+  //
+  case AMDGPU::V_SUBBREV_U32_dpp:
+  case AMDGPU::V_SUBBREV_U32_e32:
+  case AMDGPU::V_SUBBREV_U32_e64:
+  case AMDGPU::V_SUBBREV_U32_e64_dpp:
+  case AMDGPU::V_SUBBREV_U32_sdwa:
+  //
+  case AMDGPU::V_SUBB_U32_dpp:
+  case AMDGPU::V_SUBB_U32_e32:
+  case AMDGPU::V_SUBB_U32_e64:
+  case AMDGPU::V_SUBB_U32_e64_dpp:
+  case AMDGPU::V_SUBB_U32_sdwa:
+  //
+  case AMDGPU::V_SUBREV_CO_U32_dpp:
+  case AMDGPU::V_SUBREV_CO_U32_e32:
+  case AMDGPU::V_SUBREV_CO_U32_e64:
+  case AMDGPU::V_SUBREV_CO_U32_e64_dpp:
+  case AMDGPU::V_SUBREV_CO_U32_sdwa:
+  //
+  case AMDGPU::V_SUBREV_U32_dpp:
+  case AMDGPU::V_SUBREV_U32_e32:
+  case AMDGPU::V_SUBREV_U32_e64:
+  case AMDGPU::V_SUBREV_U32_e64_dpp:
+  case AMDGPU::V_SUBREV_U32_sdwa:
+  //
+  case AMDGPU::V_SUB_CO_U32_dpp:
+  case AMDGPU::V_SUB_CO_U32_e32:
+  case AMDGPU::V_SUB_CO_U32_e64:
+  case AMDGPU::V_SUB_CO_U32_e64_dpp:
+  case AMDGPU::V_SUB_CO_U32_sdwa:
+  //
+  case AMDGPU::V_SUB_U32_dpp:
+  case AMDGPU::V_SUB_U32_e32:
+  case AMDGPU::V_SUB_U32_e64:
+  case AMDGPU::V_SUB_U32_e64_dpp:
+  case AMDGPU::V_SUB_U32_sdwa:
+    return true;
+  default:
+    return false;
+  }
+}
+
 int SIInstrInfo::pseudoToMCOpcode(int Opcode) const {
   Opcode = SIInstrInfo::getNonSoftWaitcntOpcode(Opcode);
 
   unsigned Gen = subtargetEncodingFamily(ST);
 
-  if ((get(Opcode).TSFlags & SIInstrFlags::renamedInGFX9) != 0 &&
-    ST.getGeneration() == AMDGPUSubtarget::GFX9)
+  if (isRenamedInGFX9(Opcode) && ST.getGeneration() == AMDGPUSubtarget::GFX9) {
     Gen = SIEncodingFamily::GFX9;
+  }
 
   // Adjust the encoding family to GFX80 for D16 buffer instructions when the
   // subtarget has UnpackedD16VMem feature.
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.h b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
index d774826c1d08c0..21e7156cf74c9f 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
@@ -1339,6 +1339,8 @@ class SIInstrInfo final : public AMDGPUGenInstrInfo {
   /// Return true if this opcode should not be used by codegen.
   bool isAsmOnlyOpcode(int MCOp) const;
 
+  bool isRenamedInGFX9(int Opcode) const;
+
   const TargetRegisterClass *getRegClass(const MCInstrDesc &TID, unsigned OpNum,
                                          const TargetRegisterInfo *TRI,
                                          const MachineFunction &MF)
diff --git a/llvm/lib/Target/AMDGPU/VOP2Instructions.td b/llvm/lib/Target/AMDGPU/VOP2Instructions.td
index 13fe79b4759608..8c5212bb58eca4 100644
--- a/llvm/lib/Target/AMDGPU/VOP2Instructions.td
+++ b/llvm/lib/Target/AMDGPU/VOP2Instructions.td
@@ -141,26 +141,21 @@ class getVOP2Pat64 <SDPatternOperator node, VOPProfile P> : LetDummies {
 multiclass VOP2Inst_e32<string opName,
                         VOPProfile P,
                         SDPatternOperator node = null_frag,
-                        string revOp = opName,
-                        bit GFX9Renamed = 0> {
-  let renamedInGFX9 = GFX9Renamed in {
+                        string revOp = opName> {
     def _e32 : VOP2_Pseudo <opName, P, VOPPatOrNull<node,P>.ret>,
                Commutable_REV<revOp#"_e32", !eq(revOp, opName)>;
-  } // End renamedInGFX9 = GFX9Renamed
 }
 multiclass
     VOP2Inst_e32_VOPD<string opName, VOPProfile P, bits<5> VOPDOp,
                       string VOPDName, SDPatternOperator node = null_frag,
-                      string revOp = opName, bit GFX9Renamed = 0> {
-  defm NAME : VOP2Inst_e32<opName, P, node, revOp, GFX9Renamed>,
+                      string revOp = opName> {
+  defm NAME : VOP2Inst_e32<opName, P, node, revOp>,
               VOPD_Component<VOPDOp, VOPDName>;
 }
 multiclass VOP2Inst_e64<string opName,
                         VOPProfile P,
                         SDPatternOperator node = null_frag,
-                        string revOp = opName,
-                        bit GFX9Renamed = 0> {
-  let renamedInGFX9 = GFX9Renamed in {
+                        string revOp = opName> {
     def _e64 : VOP3InstBase <opName, P, node, 1>,
                Commutable_REV<revOp#"_e64", !eq(revOp, opName)>;
 
@@ -168,45 +163,37 @@ multiclass VOP2Inst_e64<string opName,
       if P.HasExtVOP3DPP then
         def _e64_dpp  : VOP3_DPP_Pseudo <opName, P>;
     } // End SubtargetPredicate = isGFX11Plus
-  } // End renamedInGFX9 = GFX9Renamed
 }
 
 multiclass VOP2Inst_sdwa<string opName,
-                         VOPProfile P,
-                         bit GFX9Renamed = 0> {
-  let renamedInGFX9 = GFX9Renamed in {
+                         VOPProfile P> {
     if P.HasExtSDWA then
       def _sdwa : VOP2_SDWA_Pseudo <opName, P>;
-  } // End renamedInGFX9 = GFX9Renamed
 }
 
 multiclass VOP2Inst<string opName,
                     VOPProfile P,
                     SDPatternOperator node = null_frag,
-                    string revOp = opName,
-                    bit GFX9Renamed = 0> :
-    VOP2Inst_e32<opName, P, node, revOp, GFX9Renamed>,
-    VOP2Inst_e64<opName, P, node, revOp, GFX9Renamed>,
-    VOP2Inst_sdwa<opName, P, GFX9Renamed> {
-  let renamedInGFX9 = GFX9Renamed in {
+                    string revOp = opName> :
+    VOP2Inst_e32<opName, P, node, revOp>,
+    VOP2Inst_e64<opName, P, node, revOp>,
+    VOP2Inst_sdwa<opName, P> {
     if P.HasExtDPP then
       def _dpp  : VOP2_DPP_Pseudo <opName, P>;
-  }
 }
 
 multiclass VOP2Inst_t16<string opName,
                         VOPProfile P,
                         SDPatternOperator node = null_frag,
-                        string revOp = opName,
-                        bit GFX9Renamed = 0> {
+                        string revOp = opName> {
   let SubtargetPredicate = NotHasTrue16BitInsts, OtherPredicates = [Has16BitInsts]  in {
-    defm NAME : VOP2Inst<opName, P, node, revOp, GFX9Renamed>;
+    defm NAME : VOP2Inst<opName, P, node, revOp>;
   }
   let SubtargetPredicate = UseRealTrue16Insts in {
-    defm _t16 : VOP2Inst<opName#"_t16", VOPProfile_True16<P>, node, revOp#"_t16", GFX9Renamed>;
+    defm _t16 : VOP2Inst<opName#"_t16", VOPProfile_True16<P>, node, revOp#"_t16">;
   }
   let SubtargetPredicate = UseFakeTrue16Insts in {
-    defm _fake16 : VOP2Inst<opName#"_fake16", VOPProfile_Fake16<P>, node, revOp#"_fake16", GFX9Renamed>;
+    defm _fake16 : VOP2Inst<opName#"_fake16", VOPProfile_Fake16<P>, node, revOp#"_fake16">;
   }
 }
 
@@ -217,13 +204,12 @@ multiclass VOP2Inst_t16<string opName,
 multiclass VOP2Inst_e64_t16<string opName,
                         VOPProfile P,
                         SDPatternOperator node = null_frag,
-                        string revOp = opName,
-                        bit GFX9Renamed = 0> {
+                        string revOp = opName> {
   let SubtargetPredicate = NotHasTrue16BitInsts, OtherPredicates = [Has16BitInsts]  in {
-    defm NAME : VOP2Inst<opName, P, node, revOp, GFX9Renamed>;
+    defm NAME : VOP2Inst<opName, P, node, revOp>;
   }
   let SubtargetPredicate = HasTrue16BitInsts in {
-    defm _t16 : VOP2Inst_e64<opName#"_t16", VOPProfile_Fake16<P>, node, revOp#"_t16", GFX9Renamed>;
+    defm _t16 : VOP2Inst_e64<opName#"_t16", VOPProfile_Fake16<P>, node, revOp#"_t16">;
   }
 }
 
@@ -232,24 +218,19 @@ multiclass VOP2Inst_VOPD<string opName,
                          bits<5> VOPDOp,
                          string VOPDName,
                          SDPatternOperator node = null_frag,
-                         string revOp = opName,
-                         bit GFX9Renamed = 0> :
-    VOP2Inst_e32_VOPD<opName, P, VOPDOp, VOPDName, node, revOp, GFX9Renamed>,
-    VOP2Inst_e64<opName, P, node, revOp, GFX9Renamed>,
-    VOP2Inst_sdwa<opName, P, GFX9Renamed> {
-  let renamedInGFX9 = GFX9Renamed in {
+                         string revOp = opName> :
+    VOP2Inst_e32_VOPD<opName, P, VOPDOp, VOPDName, node, revOp>,
+    VOP2Inst_e64<opName, P, node, revOp>,
+    VOP2Inst_sdwa<opName, P> {
     if P.HasExtDPP then
       def _dpp  : VOP2_DPP_Pseudo <opName, P>;
-  }
 }
 
 multiclass VOP2bInst <string opName,
                       VOPProfile P,
                       SDPatternOperator node = null_frag,
                       string revOp = opName,
-                      bit GFX9Renamed = 0,
                       bit useSGPRInput = !eq(P.NumSrcArgs, 3)> {
-  let renamedInGFX9 = GFX9Renamed in {
     let SchedRW = [Write32Bit, WriteSALU] in {
       let Uses = !if(useSGPRInput, [VCC, EXEC], [EXEC]), Defs = [VCC] in {
         def _e32 : VOP2_Pseudo <opName, P, VOPPatOrNull<node,P>.ret>,
@@ -273,7 +254,6 @@ multiclass VOP2bInst <string opName,
           def _e64_dpp  : VOP3_DPP_Pseudo <opName, P>;
       } // End SubtargetPredicate = isGFX11Plus
     }
-  }
 }
 
 class VOP2bInstAlias <VOP2_Pseudo ps, Instruction inst,
@@ -751,18 +731,18 @@ def V_MADAK_F32 : VOP2_Pseudo <"v_madak_f32", VOP_MADAK_F32, []>;
 
 // No patterns so that the scalar instructions are always selected.
 // The scalar versions will be replaced with vector when needed later.
-defm V_ADD_CO_U32 : VOP2bInst <"v_add_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_add_co_u32", 1>;
-defm V_SUB_CO_U32 : VOP2bInst <"v_sub_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_sub_co_u32", 1>;
-defm V_SUBREV_CO_U32 : VOP2bInst <"v_subrev_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_sub_co_u32", 1>;
-defm V_ADDC_U32 : VOP2bInst <"v_addc_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_addc_u32", 1>;
-defm V_SUBB_U32 : VOP2bInst <"v_subb_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_subb_u32", 1>;
-defm V_SUBBREV_U32 : VOP2bInst <"v_subbrev_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_subb_u32", 1>;
+defm V_ADD_CO_U32 : VOP2bInst <"v_add_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_add_co_u32">;
+defm V_SUB_CO_U32 : VOP2bInst <"v_sub_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_sub_co_u32">;
+defm V_SUBREV_CO_U32 : VOP2bInst <"v_subrev_co_u32", VOP2b_I32_I1_I32_I32, null_frag, "v_sub_co_u32">;
+defm V_ADDC_U32 : VOP2bInst <"v_addc_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_addc_u32">;
+defm V_SUBB_U32 : VOP2bInst <"v_subb_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_subb_u32">;
+defm V_SUBBREV_U32 : VOP2bInst <"v_subbrev_u32", VOP2b_I32_I1_I32_I32_I1, null_frag, "v_subb_u32">;
 
 
 let SubtargetPredicate = HasAddNoCarryInsts, isReMaterializable = 1 in {
-defm V_ADD_U32 : VOP2Inst_VOPD <"v_add_u32", VOP_I32_I32_I32_ARITH, 0x10, "v_add_nc_u32", null_frag, "v_add_u32", 1>;
-defm V_SUB_U32 : VOP2Inst <"v_sub_u32", VOP_I32_I32_I32_ARITH, null_frag, "v_sub_u32", 1>;
-defm V_SUBREV_U32 : VOP2Inst <"v_subrev_u32", VOP_I32_I32_I32_ARITH, null_frag, "v_sub_u32", 1>;
+defm V_ADD_U32 : VOP2Inst_VOPD <"v_add_u32", VOP_I32_I32_I32_ARITH, 0x10, "v_add_nc_u32", null_frag, "v_add_u32">;
+defm V_SUB_U32 : VOP2Inst <"v_sub_u32", VOP_I32_I32_I32_ARITH, null_frag, "v_sub_u32">;
+defm V_SUBREV_U32 : VOP2Inst <"v_subrev_u32", VOP_I32_I32_I32_ARITH, null_frag, "v_sub_u32">;
 }
 
 } // End isCommutable = 1
diff --git a/llvm/lib/Target/AMDGPU/VOP3Instructions.td b/llvm/lib/Target/AMDGPU/VOP3Instructions.td
index 7198a4022dae87..ae258f23c87f9a 100644
--- a/llvm/lib/Target/AMDGPU/VOP3Instructions.td
+++ b/llvm/lib/Target/AMDGPU/VOP3Instructions.td
@@ -333,35 +333,33 @@ let FPDPRounding = 1 in {
     defm V_FMA_F16 : VOP3Inst <"v_fma_f16", VOP3_Profile<VOP_F16_F16_F16_F16>, any_fma>;
   } // End Predicates = [Has16BitInsts, isGFX8Only]
 
-  let renamedInGFX9 = 1, SubtargetPredicate = isGFX9Plus in {
+  let SubtargetPredicate = isGFX9Plus in {
     defm V_DIV_FIXUP_F16_gfx9 : VOP3Inst <"v_div_fixup_f16_gfx9",
                                           VOP3_Profile<VOP_F16_F16_F16_F16, VOP3_OPSEL>, AMDGPUdiv_fixup>;
     defm V_FMA_F16_gfx9 : VOP3Inst <"v_fma_f16_gfx9", VOP3_Profile<VOP_F16_F16_F16_F16, VOP3_OPSEL>, any_fma>;
-  } // End renamedInGFX9 = 1, SubtargetPredicate = isGFX9Plus
+  } // End SubtargetPredicate = isGFX9Plus
 } // End FPDPRounding = 1
 
 let SubtargetPredicate = Has16BitInsts, isCommutable = 1 in {
 
-let renamedInGFX9 = 1 in {
-  defm V_MAD_U16 : VOP3Inst <"v_mad_u16", VOP3_Profile<VOP_I16_I16_I16_I16, VOP3_CLAMP>>;
-  defm V_MAD_I16 : VOP3Inst <"v_mad_i16", VOP3_Profile<VOP_I16_I16_I16_I16, VOP3_CLAMP>>;
-  let FPDPRounding = 1 in {
-    defm V_MAD_F16 : VOP3Inst <"v_mad_f16", VOP3_Profile<VOP_F16_F16_F16_F16>, any_fmad>;
-    let Uses = [MODE, M0, EXEC] in {
-    let OtherPredicates = [isNotGFX90APlus] in
-    // For some reason the intrinsic operands are in a different order
-    // from the instruction operands.
-    def V_INTERP_P2_F16 : VOP3Interp <"v_interp_p2_f16", VOP3_INTERP16<[f16, f32, i32, f32]>,
-           [(set f16:$vdst,
-             (int_amdgcn_interp_p2_f16 (VOP3Mods f32:$src2, i32:$src2_modifiers),
-                                       (VOP3Mods f32:$src0, i32:$src0_modifiers),
-                                       (i32 timm:$attrchan),
-                                       (i32 timm:$attr),
-                                       (i1 timm:$high),
-                                       M0))]>;
-    } // End Uses = [M0, MODE, EXEC]
-  } // End FPDPRounding = 1
-} // End renamedInGFX9 = 1
+defm V_MAD_U16 : VOP3Inst <"v_mad_u16", VOP3_Profile<VOP_I16_I16_I16_I16, VOP3_CLAMP>>;
+defm V_MAD_I16 : VOP3Inst <"v_mad_i16", VOP3_Profile<VOP_I16_I16_I16_I16, VOP3_CLAMP>>;
+let FPDPRounding = 1 in {
+  defm V_MAD_F16 : VOP3Inst <"v_mad_f16", VOP3_Profile<VOP_F16_F16_F16_F16>, any_fmad>;
+  let Uses = [MODE, M0, EXEC] in {
+  let OtherPredicates = [isNotGFX90APlus] in
+  // For some reason the intrinsic operands are in a different order
+  // from the instruction operands.
+  def V_INTERP_P2_F16 : VOP3Interp <"v_interp_p2_f16", VOP3_INTERP16<[f16, f32, i32, f32]>,
+          [(set f16:$vdst,
+            (int_amdgcn_interp_p2_f16 (VOP3Mods f32:$src2, i32:$src2_modifiers),
+                                      (VOP3Mods f32:$src0, i32:$src0_modifiers),
+                                      (i32 timm:$attrchan),
+                                      (i32 timm:$attr),
+                                      (i1 timm:$high),
+                                      M0))]>;
+  } // End Uses = [M0, MODE, EXEC]
+} // End FPDPRounding = 1
 
 let SubtargetPredicate = isGFX9Only, FPDPRounding = 1 in {
   defm V_MAD_F16_gfx9   : VOP3Inst <"v_mad_f16_gfx9", VOP3_Profile<VOP_F16_F16_F16_F16, VOP3_OPSEL>> ;

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

CRobeck · 2024-02-28T06:52:20Z

Any other comments?

CRobeck · 2024-03-01T07:52:59Z

@arsenm @rampitec is this OK to merge?

arsenm · 2024-03-01T08:06:20Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

 int SIInstrInfo::pseudoToMCOpcode(int Opcode) const {
  Opcode = SIInstrInfo::getNonSoftWaitcntOpcode(Opcode);

  unsigned Gen = subtargetEncodingFamily(ST);

-  if ((get(Opcode).TSFlags & SIInstrFlags::renamedInGFX9) != 0 &&
-    ST.getGeneration() == AMDGPUSubtarget::GFX9)
+  if (isRenamedInGFX9(Opcode) && ST.getGeneration() == AMDGPUSubtarget::GFX9) {


Suggested change

if (isRenamedInGFX9(Opcode) && ST.getGeneration() == AMDGPUSubtarget::GFX9) {

if (ST.getGeneration() == AMDGPUSubtarget::GFX9 && isRenamedInGFX9(Opcode)) {

But this moves knowledge from tablegen, into manually written functions

I agree that's a downside, but a minor one IMO:

This information is needed in a single place (still a single source of truth, it's just no longer TG)

This affects a small amount of instructions (no need to automatically generate it)

We're unlikely to add any new instructions in that category (so changing C++ isn't a bigger annoyance than changing TG)

In the end it's really a matter of taste, I personally like seeing our TableGen get simpler with minimal downsides, but I'm also okay if everyone else prefers to keep as much as possible in TableGen

Once we remove the bit from SIInstrFormats my thought was to have all the information together in the C++ file since it seems like the only place it's really needed/used. Don't have a strong opinion.

@arsenm do you feel strongly here? If so, what is the better alternative in your view? Move the isRenamedInGFX9 check back into the TableGen files?

Not really. Do we really have a pressing need to scavenge a bit right now? It's adding more complexity to the emission of every instruction

I'm not sure I ever understood this bit in the first place; why does the rename have to come up in the encoding emission? Can we avoid needing to know this in the first place here?

I'm not sure I ever understood this bit in the first place; why does the rename have to come up in the encoding emission? Can we avoid needing to know this in the first place here?

It's because GFX8 and GFX9 use the same Real for some instructions. The obvious way to support this would be to have it directly represented in the getMCOpcodeGen table, i.e. have the same value (Real) in multiple columns (EncodingFamilies) of the same row (Pseudo). But the table is defined with InstrMapping which does not let you do that.

Do we really have a pressing need to scavenge a bit right now?

I'm not sure it's pressing but @kzhuravl has an on going effect to reduce the number of TableGen bits we use and this was decided upon as one of the lower hanging opportunities.

I'm confused, InstrMapping allows you to define arbitrary columns? I think we should fix that if there's an issue

github-actions · 2024-03-25T19:31:43Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-25T19:31:43Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · 2024-05-08T17:49:59Z

Is this still necessary?

rampitec · 2024-05-08T17:58:27Z

Is this still necessary?

We are still short of these bits, even if we have solved immediate problem.

CRobeck · 2024-05-12T20:10:32Z

I'm happy to update the PR to something that we are OK merging but it wasn't clear there was a consensus on a path forward.

I'm not sure I have another solution to moving things out of the TableGen file to free up this bit slot.

CRobeck · 2024-09-25T13:00:26Z

@arsenm @rampitec What should I do with this? Can I merge it? Just close it?

rampitec · 2024-09-25T18:44:53Z

@arsenm @rampitec What should I do with this? Can I merge it? Just close it?

I never liked that bit. If @arsenm is OK with that I would go with this patch.

arsenm

There was something I wanted to check for this, but I don't remember what it was.

pseudoToMCOpcode now requires much more manual effort than should really be required. I continue to move towards thinking using all of these pseudo instructions is a mistake, and we should move towards using real instructions with subtarget specific restrictions from the beginning like every other target does

arsenm · 2024-09-25T19:54:35Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

+  case OPCODE##_e64_dpp:                                                       \
+  case OPCODE##_sdwa:
+
+static bool isRenamedInGFX9(int Opcode) {


I don't like this bit, but I don't really like needing this list of opcodes either

jayfoad · 2024-09-26T09:04:58Z

rampitec said:

I never liked that bit.

arsenm said:

pseudoToMCOpcode now requires much more manual effort than should really be required.

A better solution for this mess would be to fix the getMCOpcodeGen table to support using the same Real for different encoding families, as described above:

It's because GFX8 and GFX9 use the same Real for some instructions. The obvious way to support this would be to have it directly represented in the getMCOpcodeGen table, i.e. have the same value (Real) in multiple columns (EncodingFamilies) of the same row (Pseudo). But the table is defined with InstrMapping which does not let you do that.

Nicolai previously suggested we could do that by "extending the InstrMapping infrastructure so that column fields in TableGen can be given as lists".

…ion/macro to free up a bit slot (llvm#82787) Follow on to llvm#81525 and llvm#81901 in the series of consolidating bits in TSFlags. Remove renamedInGFX9 from SIInstrFormats.td and move to helper function/macro in SIInstrInfo. renamedInGFX9 points to V_{add, sub, subrev, addc, subb, subbrev}_ U32 and V_{div_fixup_F16, fma_F16, interp_p2_F16, mad_F16, mad_U16, mad_I16}.

CRobeck requested review from jayfoad, rampitec, kzhuravl and Pierre-vh February 23, 2024 16:31

llvmbot added the backend:AMDGPU label Feb 23, 2024

arsenm reviewed Feb 23, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

CRobeck force-pushed the users/crobeck/remove-renamedInGFX9-bit branch 2 times, most recently from 4599311 to 2b14998 Compare February 25, 2024 15:32

Pierre-vh reviewed Feb 26, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp Outdated Show resolved Hide resolved

CRobeck force-pushed the users/crobeck/remove-renamedInGFX9-bit branch from eabfff6 to a3e1951 Compare February 27, 2024 15:14

CRobeck changed the title ~~[AMDGPU] Move renamedInGFX9 from TableGen to SIInstrInfo helper function to free up a bit slot~~ [AMDGPU] Move renamedInGFX9 from TableGen to SIInstrInfo helper function/macro to free up a bit slot Feb 28, 2024

arsenm reviewed Mar 1, 2024

View reviewed changes

CRobeck requested review from aaupov, maksfb, rafaelauler, ayermolo, dcci, aartbik, PeimingLiu, yinying-lisa-li, matthias-springer, hanhanW, nicolasvasilache, dcaballe, banach-space, MaheshRavishankar and JDevlieghere as code owners March 1, 2024 08:31

CRobeck removed request for ayermolo and yinying-lisa-li March 1, 2024 08:52

CRobeck force-pushed the users/crobeck/remove-renamedInGFX9-bit branch 2 times, most recently from fafd16a to 62dc46d Compare March 1, 2024 09:11

CRobeck added 4 commits March 4, 2024 16:19

remove renamedInGFX9 bit and move check into SIInstrInfo helper function

52e6a50

formatting

a67713d

move isRenamedInGFX9 to be static and add helper macro for case prefixes

9f311a0

reorder conditional statement

2d5d5c3

CRobeck force-pushed the users/crobeck/remove-renamedInGFX9-bit branch from 62dc46d to 2d5d5c3 Compare March 4, 2024 15:19

CRobeck added 3 commits March 12, 2024 10:03

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

8cb6bb3

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

62b63b4

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

9c81908

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

6714fd0

arsenm approved these changes Sep 25, 2024

View reviewed changes

CRobeck added 3 commits September 25, 2024 18:54

fix merge conflicts

913c470

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

c67340a

Merge branch 'main' into users/crobeck/remove-renamedInGFX9-bit

67edd86

CRobeck merged commit 661666d into main Sep 26, 2024
5 of 7 checks passed

CRobeck deleted the users/crobeck/remove-renamedInGFX9-bit branch September 26, 2024 00:38

	if (isRenamedInGFX9(Opcode) && ST.getGeneration() == AMDGPUSubtarget::GFX9) {
	if (ST.getGeneration() == AMDGPUSubtarget::GFX9 && isRenamedInGFX9(Opcode)) {

[AMDGPU] Move renamedInGFX9 from TableGen to SIInstrInfo helper function/macro to free up a bit slot #82787

[AMDGPU] Move renamedInGFX9 from TableGen to SIInstrInfo helper function/macro to free up a bit slot #82787

Uh oh!

Conversation

CRobeck commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 23, 2024

Uh oh!

Uh oh!

Uh oh!

CRobeck commented Feb 28, 2024

Uh oh!

CRobeck commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CRobeck Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 25, 2024

Uh oh!

github-actions bot commented Mar 25, 2024

Uh oh!

arsenm commented May 8, 2024

Uh oh!

rampitec commented May 8, 2024

Uh oh!

CRobeck commented May 12, 2024

Uh oh!

CRobeck commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rampitec commented Sep 25, 2024

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jayfoad commented Sep 26, 2024

Uh oh!

Uh oh!

CRobeck commented Feb 23, 2024 •

edited

Loading

CRobeck commented Mar 1, 2024 •

edited

Loading

CRobeck Mar 5, 2024 •

edited

Loading

CRobeck commented Sep 25, 2024 •

edited

Loading