[AMDGPU][MC] Corrected V_*QSAD* instructions to check that dest register is different than any of the src

[dp]

Summary

The V_MQSAD_PK_U16_U8, V_QSAD_PK_U16_U8, and V_MQSAD_U32_U8 take more than 1 pass in hardware.
For these three instructions, the destination registers must be different than all sources so that the first pass does not overwrite sources for the following passes.

See Bug 33279: https://bugs.llvm.org//show_bug.cgi?id=33279

[arsenm]

I don't think it's the assembler's job to be diagnosing high level issues like this. A warning may be useful, but I don't think this should be a hard error

[artem.tamazov]

In D34003#775524, @arsenm wrote:

I don't think it's the assembler's job to be diagnosing high level issues like this. A warning may be useful, but I don't think this should be a hard error

Sounds reasonable, but still depends on the point of view. The issue is closely related to HW implementation and similar to the constant bus constraints we already diagnosing.

I think that generally, if the side effects of an instruction (i.e. the result) are not well-defined, then an error shall be flagged. Ones who really need this kind of stuff could use .byte to emit whatever strange code they want. Alternatively, we can implement an option for asm geeks which disables checks like this.

Perhaps it is worth to move further discussion (if any) to bugzilla.

[dp]

I'm going to correct this fix to avoid using a switch with a list of affected *QSAD* opcodes.
Artem suggested to use 'Constraints' information from td files to identify opcodes with

`Constraints = "@earlyclobber $vdst"`

In asm code this will look like this:

`if (Desc.getOperandConstraint(DstIdx, MCOI::EARLY_CLOBBER) != -1) { ... check limitations on dst ... }`

Does this look reasonable?

[dp]

But this will create a dependency on https://reviews.llvm.org/D33783

[artem.tamazov]

In D34003#776031, @dp wrote:

I'm going to correct this fix to avoid using a switch with a list of affected *QSAD* opcodes.

`if (Desc.getOperandConstraint(DstIdx, MCOI::EARLY_CLOBBER) != -1) { ... check limitations on dst ... }`

Does this look reasonable?

That is exactly what I have in mind, including dependency.

[dp]

Updated to identify affected opcodes using EARLY_CLOBBER constraint

llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

private:
   };
 
   bool parseSendMsgConstruct(OperandInfoTy &Msg, OperandInfoTy &Operation, int64_t &StreamId);
   bool parseHwregConstruct(OperandInfoTy &HwReg, int64_t &Offset, int64_t &Width);
 
   void errorExpTgt();
   OperandMatchResultTy parseExpTgtImpl(StringRef Str, uint8_t &Val);
 
-  bool validateOperandLimitations(const MCInst &Inst);
+  bool validateInstruction(const MCInst &Inst, const SMLoc &IDLoc);
+  bool validateConstantBusLimitations(const MCInst &Inst);
+  bool validateEarlyClobberLimitations(const MCInst &Inst);
   bool usesConstantBus(const MCInst &Inst, unsigned OpIdx);
   bool isInlineConstant(const MCInst &Inst, unsigned OpIdx) const;
   unsigned findImplicitSGPRReadInVOP(const MCInst &Inst) const;
 
   bool trySkipId(const StringRef Id);
   bool trySkipToken(const AsmToken::TokenKind Kind);
   bool skipToken(const AsmToken::TokenKind Kind, const StringRef ErrMsg);
   bool parseString(StringRef &Val, const StringRef ErrMsg = "expected a string");
bool AMDGPUAsmParser::usesConstantBus(const MCInst &Inst, unsigned OpIdx) {
   const MCOperand &MO = Inst.getOperand(OpIdx);
   if (MO.isImm()) {
     return !isInlineConstant(Inst, OpIdx);
   }
   return !MO.isReg() ||
          isSGPR(mc2PseudoReg(MO.getReg()), getContext().getRegisterInfo());
 }
 
-bool AMDGPUAsmParser::validateOperandLimitations(const MCInst &Inst) {
+bool AMDGPUAsmParser::validateConstantBusLimitations(const MCInst &Inst) {
   const unsigned Opcode = Inst.getOpcode();
   const MCInstrDesc &Desc = MII.get(Opcode);
   unsigned ConstantBusUseCount = 0;
 
   if (Desc.TSFlags &
       (SIInstrFlags::VOPC |
        SIInstrFlags::VOP1 | SIInstrFlags::VOP2 |
        SIInstrFlags::VOP3 | SIInstrFlags::VOP3P |
for (int OpIdx : OpIndices) {
         }
       }
     }
   }
 
   return ConstantBusUseCount <= 1;
 }
 
+bool AMDGPUAsmParser::validateEarlyClobberLimitations(const MCInst &Inst) {
+
+  const unsigned Opcode = Inst.getOpcode();
+  const MCInstrDesc &Desc = MII.get(Opcode);
+
+  const int DstIdx = AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::vdst);
+  if (DstIdx == -1 ||
+      Desc.getOperandConstraint(DstIdx, MCOI::EARLY_CLOBBER) == -1) {
+    return true;
+  }
+
+  const MCRegisterInfo *TRI = getContext().getRegisterInfo();
+
+  const int Src0Idx = AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src0);
+  const int Src1Idx = AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src1);
+  const int Src2Idx = AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src2);
+
+  assert(DstIdx != -1);
+  const MCOperand &Dst = Inst.getOperand(DstIdx);
+  assert(Dst.isReg());
+  const unsigned DstReg = mc2PseudoReg(Dst.getReg());
+
+  const int SrcIndices[] = { Src0Idx, Src1Idx, Src2Idx };
+
+  for (int SrcIdx : SrcIndices) {
+    if (SrcIdx == -1) break;
+    const MCOperand &Src = Inst.getOperand(SrcIdx);
+    if (Src.isReg()) {
+      const unsigned SrcReg = mc2PseudoReg(Src.getReg());
+      if (isRegIntersect(DstReg, SrcReg, TRI)) {
+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
+bool AMDGPUAsmParser::validateInstruction(const MCInst &Inst,
+                                          const SMLoc &IDLoc) {
+  if (!validateConstantBusLimitations(Inst)) {
+    Error(IDLoc,
+      "invalid operand (violates constant bus restrictions)");
+    return false;
+  }
+  if (!validateEarlyClobberLimitations(Inst)) {
+    Error(IDLoc,
+      "destination must be different than all sources");
+    return false;
+  }
+
+  return true;
+}
+
 bool AMDGPUAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                               OperandVector &Operands,
                                               MCStreamer &Out,
                                               uint64_t &ErrorInfo,
                                               bool MatchingInlineAsm) {
   MCInst Inst;
   unsigned Result = Match_Success;
   for (auto Variant : getMatchedVariants()) {
for (auto Variant : getMatchedVariants()) {
     }
     if (R == Match_Success)
       break;
   }
 
   switch (Result) {
   default: break;
   case Match_Success:
-    if (!validateOperandLimitations(Inst)) {
-      return Error(IDLoc,
-                   "invalid operand (violates constant bus restrictions)");
+    if (!validateInstruction(Inst, IDLoc)) {
+      return true;
     }
     Inst.setLoc(IDLoc);
     Out.EmitInstruction(Inst, getSTI());
     return false;
 
   case Match_MissingFeature:
     return Error(IDLoc, "instruction not supported on this GPU");
 

llvm/trunk/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h

 bool isSI(const MCSubtargetInfo &STI);
 bool isCI(const MCSubtargetInfo &STI);
 bool isVI(const MCSubtargetInfo &STI);
 bool isGFX9(const MCSubtargetInfo &STI);
 
 /// \brief Is Reg - scalar register
 bool isSGPR(unsigned Reg, const MCRegisterInfo* TRI);
 
+/// \brief Is there any intersection between registers
+bool isRegIntersect(unsigned Reg0, unsigned Reg1, const MCRegisterInfo* TRI);
+
 /// If \p Reg is a pseudo reg, return the correct hardware register given
 /// \p STI otherwise return \p Reg.
 unsigned getMCReg(unsigned Reg, const MCSubtargetInfo &STI);
 
 /// \brief Convert hardware register \p Reg to a pseudo register
 LLVM_READNONE
 unsigned mc2PseudoReg(unsigned Reg);
 

llvm/trunk/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp

 
 bool isSGPR(unsigned Reg, const MCRegisterInfo* TRI) {
   const MCRegisterClass SGPRClass = TRI->getRegClass(AMDGPU::SReg_32RegClassID);
   const unsigned FirstSubReg = TRI->getSubReg(Reg, 1);
   return SGPRClass.contains(FirstSubReg != 0 ? FirstSubReg : Reg) ||
     Reg == AMDGPU::SCC;
 }
 
+bool isRegIntersect(unsigned Reg0, unsigned Reg1, const MCRegisterInfo* TRI) {
+
+  if (Reg0 == Reg1) {
+    return true;
+  }
+
+  unsigned SubReg0 = TRI->getSubReg(Reg0, 1);
+  if (SubReg0 == 0) {
+    return TRI->getSubRegIndex(Reg1, Reg0) > 0;
+  }
+
+  for (unsigned Idx = 2; SubReg0 > 0; ++Idx) {
+    if (isRegIntersect(Reg1, SubReg0, TRI)) {
+      return true;
+    }
+    SubReg0 = TRI->getSubReg(Reg0, Idx);
+  }
+
+  return false;
+}
+
 unsigned getMCReg(unsigned Reg, const MCSubtargetInfo &STI) {
 
   switch(Reg) {
   default: break;
   case AMDGPU::FLAT_SCR:
     assert(!isSI(STI));
     return isCI(STI) ? AMDGPU::FLAT_SCR_ci : AMDGPU::FLAT_SCR_vi;
 

llvm/trunk/test/MC/AMDGPU/regression/bug28168.s

 // RUN: llvm-mc -arch=amdgcn -mcpu=bonaire -show-encoding %s | FileCheck %s --check-prefix=GCN --check-prefix=SICI --check-prefix=CI
 // RUN: llvm-mc -arch=amdgcn -mcpu=tonga -show-encoding %s | FileCheck %s --check-prefix=GCN --check-prefix=CIVI --check-prefix=VI
 
-v_mqsad_pk_u16_u8 v[0:1], s[0:1], 1, v[254:255]
-// CI: [0x00,0x00,0xe6,0xd2,0x00,0x02,0xf9,0x07]
-// VI: [0x00,0x00,0xe6,0xd1,0x00,0x02,0xf9,0x07]
+v_mqsad_pk_u16_u8 v[2:3], s[0:1], 1, v[254:255]
+// CI: [0x02,0x00,0xe6,0xd2,0x00,0x02,0xf9,0x07]
+// VI: [0x02,0x00,0xe6,0xd1,0x00,0x02,0xf9,0x07]
 
-v_qsad_pk_u16_u8 v[0:1], v[0:1], 1, s[0:1]
-// CI: [0x00,0x00,0xe4,0xd2,0x00,0x03,0x01,0x00]
-// VI: [0x00,0x00,0xe5,0xd1,0x00,0x03,0x01,0x00]
+v_qsad_pk_u16_u8 v[2:3], v[0:1], 1, s[0:1]
+// CI: [0x02,0x00,0xe4,0xd2,0x00,0x03,0x01,0x00]
+// VI: [0x02,0x00,0xe5,0xd1,0x00,0x03,0x01,0x00]

llvm/trunk/test/MC/AMDGPU/vop3-errs.s

 // RUN: not llvm-mc -arch=amdgcn -show-encoding %s 2>&1 | FileCheck %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=SI -show-encoding %s 2>&1 | FileCheck %s
 
 v_add_f32_e64 v0, v1
 // CHECK: error: too few operands for instruction
 
 v_div_scale_f32  v24, vcc, v22, 1.1, v22
 // CHECK: error: invalid operand for instruction
 
 v_mqsad_u32_u8 v[0:3], s[2:3], v4, v[0:3]
 // CHECK: error: instruction not supported on this GPU
+
+v_mqsad_pk_u16_u8 v[0:1], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[1:2], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[2:3], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[3:4], v[0:1], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[4:5], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[5:6], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[8:9], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources
+
+v_mqsad_pk_u16_u8 v[9:10], v[1:2], v9, v[4:5]
+// CHECK: error: destination must be different than all sources

llvm/trunk/test/MC/AMDGPU/vop3.s

 v_div_scale_f32 v24, vcc, v22, v22, 0xc0000000
 // SICI: v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xda,0xd2,0x16,0x2d,0xd6,0x03]
 // VI:   v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xe0,0xd1,0x16,0x2d,0xd6,0x03]
 
 v_mad_f32 v9, 0.5, v5, -v8
 // SICI: v_mad_f32 v9, 0.5, v5, -v8      ; encoding: [0x09,0x00,0x82,0xd2,0xf0,0x0a,0x22,0x84]
 // VI:   v_mad_f32 v9, 0.5, v5, -v8      ; encoding: [0x09,0x00,0xc1,0xd1,0xf0,0x0a,0x22,0x84]
 
-v_mqsad_u32_u8 v[0:3], s[2:3], v4, v[0:3]
-// CI: v_mqsad_u32_u8 v[0:3], s[2:3], v4, v[0:3] ; encoding: [0x00,0x00,0xea,0xd2,0x02,0x08,0x02,0x04]
-// VI: v_mqsad_u32_u8 v[0:3], s[2:3], v4, v[0:3] ; encoding: [0x00,0x00,0xe7,0xd1,0x02,0x08,0x02,0x04]
+v_mqsad_u32_u8 v[5:8], s[2:3], v4, v[0:3]
+// CI: v_mqsad_u32_u8 v[5:8], s[2:3], v4, v[0:3] ; encoding: [0x05,0x00,0xea,0xd2,0x02,0x08,0x02,0x04]
+// VI: v_mqsad_u32_u8 v[5:8], s[2:3], v4, v[0:3] ; encoding: [0x05,0x00,0xe7,0xd1,0x02,0x08,0x02,0x04]
 // NOSI: error: instruction not supported on this GPU
 
 v_mad_u64_u32 v[5:6], s[12:13], s1, 0, 0
 // CI: v_mad_u64_u32 v[5:6], s[12:13], s1, 0, 0 ; encoding: [0x05,0x0c,0xec,0xd2,0x01,0x00,0x01,0x02]
 // VI: v_mad_u64_u32 v[5:6], s[12:13], s1, 0, 0 ; encoding: [0x05,0x0c,0xe8,0xd1,0x01,0x00,0x01,0x02]
 // NOSI: error: instruction not supported on this GPU
 
 v_mad_i64_i32 v[5:6], s[12:13], s1, 0, v[254:255]