[AMDGPU] Add subtarget feature for MAD_U64/I64 bug on GFX11

[mbrkusanin]

Following an offline discussion I've looked into adding a subtarget feature for bug for V_MAD_U64_U32 and V_MAD_I64_I32. Since VOPReal is tied with VOPPseudo I don't see a nice way of having both options for gfx11 without also having 2 real versions.
If we don't care about having an option to disable this bug on gfx11 then we can just eliminate the other Real.

[foad]

Looks reasonable to me.

llvm/lib/Target/AMDGPU/VOP3Instructions.td

923

You shouldn't need to set OtherPredicates here. Real instructions usually copy these predicates from the Pseudo instruction.

[arsenm]

Is this still relevant? I thought there was a discussion that this wasn't actually broken?

[mbrkusanin]

• By overriding opName (or PseudoInstr of tablegen record) for _strict pseudo we can have both pseudos select into same real instruction, so now we don't need two.
• Updated use of features/predicate so now it works properly with -mattr=-mad-intra-fwd-bug which was the point of this change.

[mbrkusanin]

In D133012#3932090, @arsenm wrote:

Is this still relevant? I thought there was a discussion that this wasn't actually broken?

Nothing was really broken. It was just if we want a feature for this bug or not.

[Joe_Nash]

In D133012#3933756, @mbrkusanin wrote:

• By overriding opName (or PseudoInstr of tablegen record) for _strict pseudo we can have both pseudos select into same real instruction, so now we don't need two.

This is a creative use of the opName, but I think it probably hinders readability. No other instructions are doing this, so when I look at definition of the Real instruction it seems to refer to the non-strict pseudo.

[mbrkusanin]

In D133012#3933895, @Joe_Nash wrote:

In D133012#3933756, @mbrkusanin wrote:

• By overriding opName (or PseudoInstr of tablegen record) for _strict pseudo we can have both pseudos select into same real instruction, so now we don't need two.

This is a creative use of the opName, but I think it probably hinders readability. No other instructions are doing this, so when I look at definition of the Real instruction it seems to refer to the non-strict pseudo.

Well, it refers to both in a way. I changed the one it actually uses for initialization to be _strict pseudo which in turns uses same name as non strict one.

[Joe_Nash]

Thanks, LGTM.

llvm/lib/Target/AMDGPU/VOP3Instructions.td

300

For other types of instructions (VOPC, VOP2) opName is used as a key into mapping tables, so having duplicate entries with the same name can cause collisions. As long as there are no functional issues, I'm fine with it as is. It will be a tablegen failure if someone tries to add a mapping table and use that as the key in the future.

[mbrkusanin]

llvm/lib/Target/AMDGPU/VOP3Instructions.td

300

Is current solutions preferable to having two versions for Real instruction? Problem with having two Reals is we would need to disable one for decoding because of conflict and then manually adjust it, which does not look nice. Encoding is easily resolved with just some extra predicates.

[foad]

I have often wanted a standard way of mapping two different pseudos to the same real instruction.

[Joe_Nash]

In D133012#3934097, @foad wrote:

I have often wanted a standard way of mapping two different pseudos to the same real instruction.

I guess that is useful, but the behavior here is not always available. We are basically having multiple maps overloaded onto the same keys.
Here we are using only one map, from pseudo to real using opName (actually PseudoInstr, which is derived from opName) as the key. Other maps such as getDPPOp32 also use opName as the key. So if we always wanted to conveniently map two pseudos to one real, we would likely want to introduce several new keys that don't interfere with each other.

llvm/lib/Target/AMDGPU/VOP3Instructions.td

300

I don't quite understand what you are asking. Currently we have 2 pseudos and one real for each subtarget. We should not need multiple reals on a single subtarget. The way to resolve my comment fully is to go to the following. Which is NFC currently.

defm V_MAD_U64_U32_strict : VOP3Inst <"v_mad_u64_u32_strict", VOP3b_I64_I1_I32_I32_I64>;
defm V_MAD_I64_I32_strict : VOP3Inst <"v_mad_i64_i32_strict", VOP3b_I64_I1_I32_I32_I64>;

[mbrkusanin]

llvm/lib/Target/AMDGPU/VOP3Instructions.td

300

Actually it would not be NFC. Mapping non-strict pseudo to real would fail for: llc -mcpu=gfx1100 -mattr=-mad-intra-fwd-bug.
The point of the patch is to have an option to disable the workaround for the bug.

Maybe I misunderstood what the first comment meant. Patch as-is matches both Pseudos into same Real. Is the potential issue for a new mapping table in the future if that table wants to use opName as key? Or are you talking about mapping Reals back into Pseudos?
Would ,,let PseudoInstr = " for pseudo make a difference (opName would be different then)?

[arsenm]

LGTM but I don't understand the name change

llvm/lib/Target/AMDGPU/VOP3Instructions.td

299–300

I don't understand the name change from _gfx11 to _strict

[mbrkusanin]

llvm/lib/Target/AMDGPU/VOP3Instructions.td

299–300

It's strange to have _gfx11 on a pseudo and then a real _gfx11_e64_gfx11. Should I restore it?

[arsenm]

llvm/lib/Target/AMDGPU/VOP3Instructions.td

299–300

That's consistent with other _gfx* behavior changing instruction variants.

I do think we have a sustainability problem with all the semantic changes of the same opcodes. Over time I've started to think it would be better to codegen to concrete opcodes and swap out the instruction tables per-sub target or something

[mbrkusanin]

• reverted name change
• rebase

[Joe_Nash]

llvm/lib/Target/AMDGPU/VOP3Instructions.td

300

Actually it would not be NFC. Mapping non-strict pseudo to real would fail for: llc -mcpu=gfx1100 -mattr=-mad-intra-fwd-bug.
The point of the patch is to have an option to disable the workaround for the bug.

Ah, I missed this part where you wanted to turn off the feature on gfx11. Perhaps there should be a test for that.

Maybe I misunderstood what the first comment meant. Patch as-is matches both Pseudos into same Real. Is the potential issue for a new mapping table in the future if that table wants to use opName as key? Or are you talking about mapping Reals back into Pseudos?
Would ,,let PseudoInstr = " for pseudo make a difference (opName would be different then)?

"a new mapping table in the future if that table wants to use opName as key" is the part I was worried about. It's OK to leave it for now and address it later if it becomes a problem.

[arsenm]

Last I checked you can't actually turn off a subtarget feature that's attached to the cpu definition. It has some insane behavior where it concludes you didn't really mean that processor and turns off all the features

llvm/lib/Target/AMDGPU/AMDGPU.td

 >;
 
 def FeatureImageGather4D16Bug : SubtargetFeature<"image-gather4-d16-bug",
   "HasImageGather4D16Bug",
   "true",
   "Image Gather4 D16 hardware bug"
 >;
 
+def FeatureMADIntraFwdBug : SubtargetFeature<"mad-intra-fwd-bug",
+  "HasMADIntraFwdBug",
+  "true",
+  "MAD_U64/I64 intra instruction forwarding bug"
+>;
+
 class SubtargetFeatureLDSBankCount <int Value> : SubtargetFeature <
   "ldsbankcount"#Value,
   "LDSBankCount",
   !cast<string>(Value),
   "The number of LDS banks per compute unit."
 >;
 
 def FeatureLDSBankCount16 : SubtargetFeatureLDSBankCount<16>;
def FeatureISAVersion11_Common : FeatureSet<
    FeatureShaderCyclesRegister,
    FeatureArchitectedFlatScratch,
    FeatureAtomicFaddRtnInsts,
    FeatureAtomicFaddNoRtnInsts,
    FeatureFlatAtomicFaddF32Inst,
    FeatureImageInsts,
    FeaturePackedTID,
    FeatureVcmpxPermlaneHazard,
-   FeatureBackOffBarrier]>;
+   FeatureBackOffBarrier,
+   FeatureMADIntraFwdBug]>;
 
 def FeatureISAVersion11_0_0 : FeatureSet<
   !listconcat(FeatureISAVersion11_Common.Features,
     [FeatureGFX11FullVGPRs,
      FeatureUserSGPRInit16Bug])>;
 
 def FeatureISAVersion11_0_1 : FeatureSet<
   !listconcat(FeatureISAVersion11_Common.Features,

 
 def EnableFlatScratch : Predicate<"Subtarget->enableFlatScratch()">;
 
 def DisableFlatScratch : Predicate<"!Subtarget->enableFlatScratch()">;
 
 def HasUnalignedAccessMode : Predicate<"Subtarget->hasUnalignedAccessMode()">,
   AssemblerPredicate<(all_of FeatureUnalignedAccessMode)>;
 
+def HasMADIntraFwdBug : Predicate<"Subtarget->hasMADIntraFwdBug()">;
+
+def HasNotMADIntraFwdBug : Predicate<"!Subtarget->hasMADIntraFwdBug()">;
+
 // Include AMDGPU TD files
 include "SISchedule.td"
 include "GCNProcessors.td"
 include "AMDGPUInstrInfo.td"
 include "SIRegisterInfo.td"
 include "AMDGPURegisterBanks.td"
 include "AMDGPUInstructions.td"
 include "SIInstrInfo.td"
 include "AMDGPUCallingConv.td"
 include "AMDGPUSearchableTables.td"

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

 }
 
 // We need to handle this here because tablegen doesn't support matching
 // instructions with multiple outputs.
 void AMDGPUDAGToDAGISel::SelectMAD_64_32(SDNode *N) {
   SDLoc SL(N);
   bool Signed = N->getOpcode() == AMDGPUISD::MAD_I64_I32;
   unsigned Opc;
-  if (Subtarget->getGeneration() == AMDGPUSubtarget::GFX11)
+  if (Subtarget->hasMADIntraFwdBug())
     Opc = Signed ? AMDGPU::V_MAD_I64_I32_gfx11_e64
                  : AMDGPU::V_MAD_U64_U32_gfx11_e64;
   else
     Opc = Signed ? AMDGPU::V_MAD_I64_I32_e64 : AMDGPU::V_MAD_U64_U32_e64;
 
   SDValue Clamp = CurDAG->getTargetConstant(0, SL, MVT::i1);
   SDValue Ops[] = { N->getOperand(0), N->getOperand(1), N->getOperand(2),
                     Clamp };
   CurDAG->SelectNodeTo(N, Opc, N->getVTList(), Ops);
 }
 
 // We need to handle this here because tablegen doesn't support matching
 // instructions with multiple outputs.
 void AMDGPUDAGToDAGISel::SelectMUL_LOHI(SDNode *N) {
   SDLoc SL(N);
   bool Signed = N->getOpcode() == ISD::SMUL_LOHI;
   unsigned Opc;
-  if (Subtarget->getGeneration() == AMDGPUSubtarget::GFX11)
+  if (Subtarget->hasMADIntraFwdBug())
     Opc = Signed ? AMDGPU::V_MAD_I64_I32_gfx11_e64
                  : AMDGPU::V_MAD_U64_U32_gfx11_e64;
   else
     Opc = Signed ? AMDGPU::V_MAD_I64_I32_e64 : AMDGPU::V_MAD_U64_U32_e64;
 
   SDValue Zero = CurDAG->getTargetConstant(0, SL, MVT::i64);
   SDValue Clamp = CurDAG->getTargetConstant(0, SL, MVT::i1);
   SDValue Ops[] = {N->getOperand(0), N->getOperand(1), Zero, Clamp};

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

 
 bool AMDGPUInstructionSelector::selectG_AMDGPU_MAD_64_32(
     MachineInstr &I) const {
   MachineBasicBlock *BB = I.getParent();
   MachineFunction *MF = BB->getParent();
   const bool IsUnsigned = I.getOpcode() == AMDGPU::G_AMDGPU_MAD_U64_U32;
 
   unsigned Opc;
-  if (Subtarget->getGeneration() == AMDGPUSubtarget::GFX11)
+  if (Subtarget->hasMADIntraFwdBug())
     Opc = IsUnsigned ? AMDGPU::V_MAD_U64_U32_gfx11_e64
                      : AMDGPU::V_MAD_I64_I32_gfx11_e64;
   else
     Opc = IsUnsigned ? AMDGPU::V_MAD_U64_U32_e64 : AMDGPU::V_MAD_I64_I32_e64;
   I.setDesc(TII.get(Opc));
   I.addOperand(*MF, MachineOperand::CreateImm(0));
   I.addImplicitDefUseOperands(*MF);
   return constrainSelectedInstRegOperands(I, TII, TRI, RBI);

llvm/lib/Target/AMDGPU/GCNSubtarget.h

protected:
   bool HasLdsBranchVmemWARHazard = false;
   bool HasNSAtoVMEMBug = false;
   bool HasNSAClauseBug = false;
   bool HasOffset3fBug = false;
   bool HasFlatSegmentOffsetBug = false;
   bool HasImageStoreD16Bug = false;
   bool HasImageGather4D16Bug = false;
   bool HasGFX11FullVGPRs = false;
+  bool HasMADIntraFwdBug = false;
   bool HasVOPDInsts = false;
 
   // Dummy feature to use for assembler in tablegen.
   bool FeatureDisable = false;
 
   SelectionDAGTargetInfo TSInfo;
 private:
   SIInstrInfo InstrInfo;
public:
   bool hasOffset3fBug() const {
     return HasOffset3fBug;
   }
 
   bool hasImageStoreD16Bug() const { return HasImageStoreD16Bug; }
 
   bool hasImageGather4D16Bug() const { return HasImageGather4D16Bug; }
 
+  bool hasMADIntraFwdBug() const { return HasMADIntraFwdBug; }
+
   bool hasNSAEncoding() const { return HasNSAEncoding; }
 
   unsigned getNSAMaxSize() const { return NSAMaxSize; }
 
   bool hasGFX10_AEncoding() const {
     return GFX10_AEncoding;
   }
 

llvm/lib/Target/AMDGPU/VOP3Instructions.td

 
 let SubtargetPredicate = isGFX7Plus in {
 let Constraints = "@earlyclobber $vdst", SchedRW = [WriteQuarterRate32] in {
 defm V_QSAD_PK_U16_U8 : VOP3Inst <"v_qsad_pk_u16_u8", VOP3_Profile<VOP_I64_I64_I32_I64, VOP3_CLAMP>>;
 defm V_MQSAD_U32_U8 : VOP3Inst <"v_mqsad_u32_u8", VOPProfileMQSAD>;
 } // End Constraints = "@earlyclobber $vdst", SchedRW = [WriteQuarterRate32]
 } // End SubtargetPredicate = isGFX7Plus
 
-let isCommutable = 1 in {
-let SchedRW = [WriteIntMul, WriteSALU] in {
-let SubtargetPredicate = isGFX7GFX8GFX9GFX10 in {
+let isCommutable = 1, SchedRW = [WriteIntMul, WriteSALU] in {
+  let SubtargetPredicate = isGFX7Plus, OtherPredicates = [HasNotMADIntraFwdBug] in {
     defm V_MAD_U64_U32 : VOP3Inst <"v_mad_u64_u32", VOP3b_I64_I1_I32_I32_I64>;
     defm V_MAD_I64_I32 : VOP3Inst <"v_mad_i64_i32", VOP3b_I64_I1_I32_I32_I64>;
   }
-let SubtargetPredicate = isGFX11Only, Constraints = "@earlyclobber $vdst" in {
-defm V_MAD_U64_U32_gfx11 : VOP3Inst <"v_mad_u64_u32_gfx11", VOP3b_I64_I1_I32_I32_I64>;
-defm V_MAD_I64_I32_gfx11 : VOP3Inst <"v_mad_i64_i32_gfx11", VOP3b_I64_I1_I32_I32_I64>;
-} // End SubtargetPredicate = isGFX11Only, Constraints = "@earlyclobber $vdst"
-} // End SchedRW = [WriteIntMul, WriteSALU]
-} // End isCommutable = 1
+  let SubtargetPredicate = isGFX11Only, OtherPredicates = [HasMADIntraFwdBug],
+      Constraints = "@earlyclobber $vdst" in {
+    defm V_MAD_U64_U32_gfx11 : VOP3Inst <"v_mad_u64_u32", VOP3b_I64_I1_I32_I32_I64>;
+    defm V_MAD_I64_I32_gfx11 : VOP3Inst <"v_mad_i64_i32", VOP3b_I64_I1_I32_I32_I64>;
+  }
+} // End isCommutable = 1, SchedRW = [WriteIntMul, WriteSALU]
 
 
 let FPDPRounding = 1 in {
   let Predicates = [Has16BitInsts, isGFX8Only] in {
     defm V_DIV_FIXUP_F16 : VOP3Inst <"v_div_fixup_f16", VOP3_Profile<VOP_F16_F16_F16_F16>, AMDGPUdiv_fixup>;
     defm V_FMA_F16 : VOP3Inst <"v_fma_f16", VOP3_Profile<VOP_F16_F16_F16_F16>, any_fma>;
   } // End Predicates = [Has16BitInsts, isGFX8Only]
 
multiclass IMAD32_Pats <VOP3_Pseudo inst> {
   // FIXME: GlobalISel pattern exporter fails to export a pattern like this and asserts,
   // make it SDAG only.
   def : GCNPat <
         (ThreeOpFragSDAG<mul, add> i32:$src0, i32:$src1, (i32 imm:$src2)),
         (EXTRACT_SUBREG (inst $src0, $src1, (i64 (as_i64imm $src2)), 0 /* clamp */), sub0)
         >;
 }
 
-let SubtargetPredicate = isGFX9GFX10 in // exclude pre-GFX9 where it was slow
+// exclude pre-GFX9 where it was slow
+let OtherPredicates = [HasNotMADIntraFwdBug], SubtargetPredicate = isGFX9Plus in
   defm : IMAD32_Pats<V_MAD_U64_U32_e64>;
-let SubtargetPredicate = isGFX11Only in
+let OtherPredicates = [HasMADIntraFwdBug], SubtargetPredicate = isGFX11Only in
   defm : IMAD32_Pats<V_MAD_U64_U32_gfx11_e64>;
 
 def VOP3_PERMLANE_Profile : VOP3_Profile<VOPProfile <[i32, i32, i32, i32]>, VOP3_OPSEL> {
   let InsVOP3OpSel = (ins IntOpSelMods:$src0_modifiers, VRegSrc_32:$src0,
                           IntOpSelMods:$src1_modifiers, SSrc_b32:$src1,
                           IntOpSelMods:$src2_modifiers, SSrc_b32:$src2,
                           VGPR_32:$vdst_in, op_sel0:$op_sel);
   let HasClamp = 0;
   let HasExtVOP3DPP = 0;

 defm V_MINMAX_U32          : VOP3_Realtriple_gfx11<0x263>;
 defm V_MAXMIN_I32          : VOP3_Realtriple_gfx11<0x264>;
 defm V_MINMAX_I32          : VOP3_Realtriple_gfx11<0x265>;
 defm V_DOT2_F16_F16        : VOP3Dot_Realtriple_gfx11<0x266>;
 defm V_DOT2_BF16_BF16      : VOP3Dot_Realtriple_gfx11<0x267>;
 defm V_DIV_SCALE_F32       : VOP3be_Real_gfx11<0x2fc, "V_DIV_SCALE_F32", "v_div_scale_f32">;
 defm V_DIV_SCALE_F64       : VOP3be_Real_gfx11<0x2fd, "V_DIV_SCALE_F64", "v_div_scale_f64">;
 defm V_MAD_U64_U32_gfx11   : VOP3be_Real_gfx11<0x2fe, "V_MAD_U64_U32_gfx11", "v_mad_u64_u32">;
 defm V_MAD_I64_I32_gfx11   : VOP3be_Real_gfx11<0x2ff, "V_MAD_I64_I32_gfx11", "v_mad_i64_i32">;
 defm V_ADD_NC_U16          : VOP3Only_Realtriple_gfx11<0x303>;
 defm V_SUB_NC_U16          : VOP3Only_Realtriple_gfx11<0x304>;
 defm V_MUL_LO_U16_t16      : VOP3Only_Realtriple_t16_gfx11<0x305, "v_mul_lo_u16">;
 defm V_CVT_PK_I16_F32      : VOP3_Realtriple_gfx11<0x306>;
 defm V_CVT_PK_U16_F32      : VOP3_Realtriple_gfx11<0x307>;
 defm V_MAX_U16_t16         : VOP3Only_Realtriple_t16_gfx11<0x309, "v_max_u16">;
 defm V_MAX_I16_t16         : VOP3Only_Realtriple_t16_gfx11<0x30a, "v_max_i16">;
 defm V_MIN_U16_t16         : VOP3Only_Realtriple_t16_gfx11<0x30b, "v_min_u16">;