[AMDGPU] Fix use of HasModifiers in VopProfile

[mbrkusanin]

Summary

HasModifiers should be true if at least one modifier is used.
This should make the use of this field bit more consistent.

[mbrkusanin]

I would prefer to include HasClamp inside "field bit HasModifiers =" but VOPProfile allows clamp to be explicitly enabled through "bit _EnableClamp = 0" which still messes up the logic a little bit (like in class getIns64). Not sure if there is some other reason to keeping clamp separate from other modifiers.

This is why there are still a few "let HasClamp = 1;" lines. Ideally we would only want "= 0" for special cases where some modifiers are not used.

[foad]

"HasModifiers should not be true if at least one modifier is used."

Do you mean "*should* be true"?

[mbrkusanin]

In D94795#2501495, @foad wrote:

"HasModifiers should not be true if at least one modifier is used."

Do you mean "*should* be true"?

Yes. Thanks.

[arsenm]

Do we really need HasModifiers if breaking this down to src modifiers and omod?

[Joe_Nash]

I'm getting failing tests in check-llvm-mc-disassembler-amdgpu when applying this patch, can you take a look at that please?

[mbrkusanin]

• Undid few changes to fix disassemble tests.

[mbrkusanin]

In D94795#2501775, @arsenm wrote:

Do we really need HasModifiers if breaking this down to src modifiers and omod?

This
"field bit HasSrc0Mods = HasModifiers;"
basically tells us that we can replace all uses of HasModifiers with HasSrc0Mods. I guess it's more of a question of readability.

[Joe_Nash]

llvm/lib/Target/AMDGPU/SIInstrInfo.td

2071

It would be even more consistent if HasClamp was here too, so that HasModifiers is true if HasClamp is true. However, I think that can be a separate patch.

llvm/lib/Target/AMDGPU/SIInstrInfo.td

class VOPProfile <list<ValueType> _ArgVT, bit _EnableF32SrcMods = 0,
   field bit HasDst = !ne(DstVT.Value, untyped.Value);
   field bit HasDst32 = HasDst;
   field bit EmitDst = HasDst; // force dst encoding, see v_movreld_b32 special case
   field int NumSrcArgs = getNumSrcArgs<Src0VT, Src1VT, Src2VT>.ret;
   field bit HasSrc0 = !ne(Src0VT.Value, untyped.Value);
   field bit HasSrc1 = !ne(Src1VT.Value, untyped.Value);
   field bit HasSrc2 = !ne(Src2VT.Value, untyped.Value);
 
-  // TODO: Modifiers logic is somewhat adhoc here, to be refined later
-  // HasModifiers affects the normal and DPP encodings. We take note of EnableF32SrcMods, which
-  // enables modifiers for i32 type.
-  field bit HasModifiers = !or(isModifierType<Src0VT>.ret, EnableF32SrcMods);
-
   // HasSrc*FloatMods affects the SDWA encoding. We ignore EnableF32SrcMods.
   field bit HasSrc0FloatMods = isFloatType<Src0VT>.ret;
   field bit HasSrc1FloatMods = isFloatType<Src1VT>.ret;
   field bit HasSrc2FloatMods = isFloatType<Src2VT>.ret;
 
   // HasSrc*IntMods affects the SDWA encoding. We ignore EnableF32SrcMods.
   field bit HasSrc0IntMods = isIntType<Src0VT>.ret;
   field bit HasSrc1IntMods = isIntType<Src1VT>.ret;
   field bit HasSrc2IntMods = isIntType<Src2VT>.ret;
 
-  field bit HasSrc0Mods = HasModifiers;
-  field bit HasSrc1Mods = !if(HasModifiers, !or(HasSrc1FloatMods, HasSrc1IntMods), 0);
-  field bit HasSrc2Mods = !if(HasModifiers, !or(HasSrc2FloatMods, HasSrc2IntMods), 0);
-
   field bit HasClamp = !or(isModifierType<Src0VT>.ret, EnableClamp);
   field bit HasSDWAClamp = EmitDst;
   field bit HasFPClamp = !and(isFloatType<DstVT>.ret, HasClamp);
   field bit HasIntClamp = !if(isFloatType<DstVT>.ret, 0, HasClamp);
   field bit HasClampLo = HasClamp;
   field bit HasClampHi = !and(isPackedType<DstVT>.ret, HasClamp);
   field bit HasHigh = 0;
 
   field bit IsPacked = isPackedType<Src0VT>.ret;
   field bit HasOpSel = IsPacked;
   field bit HasOMod = !if(HasOpSel, 0, isFloatType<DstVT>.ret);
   field bit HasSDWAOMod = isFloatType<DstVT>.ret;
 
+  field bit HasModifiers = !or(isModifierType<Src0VT>.ret,
+                               isModifierType<Src1VT>.ret,
+                               isModifierType<Src2VT>.ret,
+                               HasOMod,
+                               EnableF32SrcMods);
+
+  field bit HasSrc0Mods = HasModifiers;
+  field bit HasSrc1Mods = !if(HasModifiers, !or(HasSrc1FloatMods, HasSrc1IntMods), 0);
+  field bit HasSrc2Mods = !if(HasModifiers, !or(HasSrc2FloatMods, HasSrc2IntMods), 0);
+
   field bit HasExt = getHasExt<NumSrcArgs, DstVT, Src0VT, Src1VT>.ret;
   field bit HasExtDPP = getHasDPP<NumSrcArgs, DstVT, Src0VT, Src1VT>.ret;
   field bit HasExtSDWA = HasExt;
   field bit HasExtSDWA9 = HasExt;
   field int NeedPatGen = PatGenMode.NoPattern;
 
   field bit IsMAI = 0;
   field bit IsDOT = 0;

llvm/lib/Target/AMDGPU/VOP1Instructions.td

 class VOPProfileI2F<ValueType dstVt, ValueType srcVt> :
   VOPProfile<[dstVt, srcVt, untyped, untyped]> {
 
   let Ins64 = (ins Src0RC64:$src0, clampmod:$clamp, omod:$omod);
   let Asm64 = "$vdst, $src0$clamp$omod";
 
   let HasModifiers = 0;
   let HasClamp = 1;
-  let HasOMod = 1;
 }
 
 def VOP1_F64_I32 : VOPProfileI2F <f64, i32>;
 def VOP1_F32_I32 : VOPProfileI2F <f32, i32>;
 def VOP1_F16_I16 : VOPProfileI2F <f16, i16>;
 
 //===----------------------------------------------------------------------===//
 // VOP1 Instructions

llvm/lib/Target/AMDGPU/VOP2Instructions.td

 def VOP_MAC_F16 : VOP_MAC <f16>;
 def VOP_MAC_F32 : VOP_MAC <f32>;
 let HasExtDPP = 0 in
 def VOP_MAC_LEGACY_F32 : VOP_MAC <f32>;
 
 class VOP_DOT_ACC<ValueType vt0, ValueType vt1> : VOP_MAC<vt0, vt1> {
   let HasClamp = 0;
   let HasExtSDWA = 0;
-  let HasModifiers = 1;
   let HasOpSel = 0;
   let IsPacked = 0;
 }
 
 def VOP_DOT_ACC_F32_V2F16 : VOP_DOT_ACC<f32, v2f16> {
   let Src0ModDPP = FPVRegInputMods;
   let Src1ModDPP = FPVRegInputMods;
 }
-def VOP_DOT_ACC_I32_I32   : VOP_DOT_ACC<i32, i32>;
+
+def VOP_DOT_ACC_I32_I32   : VOP_DOT_ACC<i32, i32> {
+  let HasSrc0Mods = 1;
+  let HasSrc1Mods = 1;
+}
 
 // Write out to vcc or arbitrary SGPR.
 def VOP2b_I32_I1_I32_I32 : VOPProfile<[i32, i32, i32, untyped], 0, /*EnableClamp=*/1> {
   let Asm32 = "$vdst, vcc, $src0, $src1";
   let Asm64 = "$vdst, $sdst, $src0, $src1$clamp";
   let AsmSDWA = "$vdst, vcc, $src0_modifiers, $src1_modifiers$clamp $dst_sel $dst_unused $src0_sel $src1_sel";
   let AsmSDWA9 = "$vdst, vcc, $src0_modifiers, $src1_modifiers$clamp $dst_sel $dst_unused $src0_sel $src1_sel";
   let AsmDPP = "$vdst, vcc, $src0, $src1 $dpp_ctrl$row_mask$bank_mask$bound_ctrl";

llvm/lib/Target/AMDGPU/VOP3Instructions.td

 
 class VOP3_Profile<VOPProfile P, VOP3Features Features = VOP3_REGULAR> : VOPProfile<P.ArgVT> {
 
   let HasClamp = !if(Features.HasClamp, 1, P.HasClamp);
   let HasOpSel = !if(Features.HasOpSel, 1, P.HasOpSel);
   let IsMAI    = !if(Features.IsMAI,    1, P.IsMAI);
   let IsPacked = !if(Features.IsPacked, 1, P.IsPacked);
 
-  let HasModifiers = !if(Features.IsPacked, !not(Features.IsMAI), P.HasModifiers);
+  let HasModifiers = !if(Features.IsMAI, 0, !or(Features.IsPacked, P.HasModifiers));
 
   // FIXME: Hack to stop printing _e64
   let Outs64 = (outs DstRC.RegClass:$vdst);
   let Asm64 =
     " " # !if(Features.HasOpSel,
               getAsmVOP3OpSel<NumSrcArgs,
                               HasIntClamp,
                               P.HasOMod,

 def VOP3_INTERP_MOV : VOPProfile<[f32, i32, i32, untyped]> {
   let Ins64 = (ins InterpSlot:$src0,
                    Attr:$attr, AttrChan:$attrchan,
                    clampmod0:$clamp, omod0:$omod);
 
   let Asm64 = "$vdst, $src0, $attr$attrchan$clamp$omod";
 
   let HasClamp = 1;
+  let HasSrc0Mods = 0;
 }
 
 class getInterp16Asm <bit HasSrc2, bit HasOMod> {
   string src2 = !if(HasSrc2, ", $src2_modifiers", "");
   string omod = !if(HasOMod, "$omod", "");
   string ret =
     " $vdst, $src0_modifiers, $attr$attrchan"#src2#"$high$clamp"#omod;
 }
def VOP3_PERMLANE_Profile : VOP3_Profile<VOPProfile <[i32, i32, i32, i32]>, VOP3_OPSEL> {
   let Src0RC64 = VRegSrc_32;
   let Src1RC64 = SCSrc_b32;
   let Src2RC64 = SCSrc_b32;
   let InsVOP3OpSel = (ins IntOpSelMods:$src0_modifiers, VRegSrc_32:$src0,
                           IntOpSelMods:$src1_modifiers, SCSrc_b32:$src1,
                           IntOpSelMods:$src2_modifiers, SCSrc_b32:$src2,
                           VGPR_32:$vdst_in, op_sel0:$op_sel);
   let HasClamp = 0;
-  let HasOMod = 0;
 }
 
 class PermlanePat<SDPatternOperator permlane,
   Instruction inst> : GCNPat<
   (permlane i32:$vdst_in, i32:$src0, i32:$src1, i32:$src2,
             timm:$fi, timm:$bc),
   (inst (as_i1timm $fi), VGPR_32:$src0, (as_i1timm $bc),
         SCSrc_b32:$src1, 0, SCSrc_b32:$src2, VGPR_32:$vdst_in)

llvm/lib/Target/AMDGPU/VOP3PInstructions.td

class VOPProfileMAI<VOPProfile P, RegisterOperand _SrcRC, RegisterOperand _DstRC,
                     RegisterOperand SrcABRC = AVSrc_32>
   : VOP3_Profile<P, VOP3_MAI> {
   let DstRC = _DstRC;
   let Src0RC64 = SrcABRC;
   let Src1RC64 = SrcABRC;
   let Src2RC64 = _SrcRC;
   let HasOpSel = 0;
   let HasClamp = 0;
-  let HasModifiers = 0;
   let Asm64 = " $vdst, $src0, $src1, $src2$cbsz$abid$blgp";
   let Ins64 = (ins Src0RC64:$src0, Src1RC64:$src1, Src2RC64:$src2, cbsz:$cbsz, abid:$abid, blgp:$blgp);
 }
 
 def VOPProfileMAI_F32_F32_X4    : VOPProfileMAI<VOP_V4F32_F32_F32_V4F32,       AISrc_128_f32,  ADst_128>;
 def VOPProfileMAI_F32_F32_X16   : VOPProfileMAI<VOP_V16F32_F32_F32_V16F32,     AISrc_512_f32,  ADst_512>;
 def VOPProfileMAI_F32_F32_X32   : VOPProfileMAI<VOP_V32F32_F32_F32_V32F32,     AISrc_1024_f32, ADst_1024>;
 def VOPProfileMAI_I32_I32_X4    : VOPProfileMAI<VOP_V4I32_I32_I32_V4I32,       AISrc_128_b32,  ADst_128>;