RFD: AMDGPU: Stop validating earlyclobber operands in assembler

[foad]

Summary

This validation was introduced in D34003 for v_qsad/v_mqsad instructions
but it applies to all instructions with earlyclobber operands, which now
includes v_mad_i64/v_mad_u64.

In all these cases I do not think there is documentation saying that the
destination must not overlap the sources. Rather there are *some* cases
where the instruction may not function correctly if there is an overlap,
and we are using earlyclobber as a conservative way of preventing
codegen from generating those cases.

I think it is unhelpful for the assembler to enforce the earlyclobber
restriction because it prevents assembling cases where the programmer
knows that in fact the overlap is safe.

See also: https://github.com/llvm/llvm-project/issues/57610

[dp]

LGTM

[foad]

In D134272#3802809, @dp wrote:

LGTM

Thanks. I'd like to get some more opinions (at least @rampitec's) before landing this.

[dp]

I asked shader writers (Ilya Perminov in particular), and they are happy with your change.

[rampitec]

Why not keep this validation and limit it to specific opcodes? For example multiplass DGEMM has the same restriction for the same reason.

[arsenm]

In D134272#3803303, @rampitec wrote:

Why not keep this validation and limit it to specific opcodes? For example multiplass DGEMM has the same restriction for the same reason.

Not only opcodes, but the specifically invalid overlaps

[rampitec]

In D134272#3803307, @arsenm wrote:

In D134272#3803303, @rampitec wrote:

Why not keep this validation and limit it to specific opcodes? For example multiplass DGEMM has the same restriction for the same reason.

Not only opcodes, but the specifically invalid overlaps

Right, it affects specific operands only, we just have no way to express it in the td.

[foad]

In D134272#3803400, @rampitec wrote:

In D134272#3803307, @arsenm wrote:

In D134272#3803303, @rampitec wrote:

Why not keep this validation and limit it to specific opcodes? For example multiplass DGEMM has the same restriction for the same reason.

Not only opcodes, but the specifically invalid overlaps

Right, it affects specific operands only, we just have no way to express it in the td.

Adding checks for specific operands of specific (DGEMM?) opcodes sounds great, if the restrictions are clearly documented, but I don't think it belongs in this patch.

For mad/qsad/mqsad I don't think there are any clearly documented restrictions, just some reports of hardware bugs that we try to work around in codegen, so I don't think the assembler should be enforcing any restrictions.

So I'd still like to proceed with this patch.

[rampitec]

OK, let's remove this and add a more specific verification later if needed.

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

private:
   SMLoc getConstLoc(const OperandVector &Operands) const;
   SMLoc getInstLoc(const OperandVector &Operands) const;
 
   bool validateInstruction(const MCInst &Inst, const SMLoc &IDLoc, const OperandVector &Operands);
   bool validateFlatOffset(const MCInst &Inst, const OperandVector &Operands);
   bool validateSMEMOffset(const MCInst &Inst, const OperandVector &Operands);
   bool validateSOPLiteral(const MCInst &Inst) const;
   bool validateConstantBusLimitations(const MCInst &Inst, const OperandVector &Operands);
-  bool validateEarlyClobberLimitations(const MCInst &Inst, const OperandVector &Operands);
   bool validateIntClampSupported(const MCInst &Inst);
   bool validateMIMGAtomicDMask(const MCInst &Inst);
   bool validateMIMGGatherDMask(const MCInst &Inst);
   bool validateMovrels(const MCInst &Inst, const OperandVector &Operands);
   bool validateMIMGDataSize(const MCInst &Inst, const SMLoc &IDLoc);
   bool validateMIMGAddrSize(const MCInst &Inst);
   bool validateMIMGD16(const MCInst &Inst);
   bool validateMIMGDim(const MCInst &Inst);
bool AMDGPUAsmParser::validateConstantBusLimitations(
 
   SMLoc LitLoc = getLitLoc(Operands);
   SMLoc RegLoc = getRegLoc(LastSGPR, Operands);
   SMLoc Loc = (LitLoc.getPointer() < RegLoc.getPointer()) ? RegLoc : LitLoc;
   Error(Loc, "invalid operand (violates constant bus restrictions)");
   return false;
 }
 
-bool
-AMDGPUAsmParser::validateEarlyClobberLimitations(const MCInst &Inst,
-                                                 const OperandVector &Operands) {
-  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 int SrcIndices[] = { Src0Idx, Src1Idx, Src2Idx };
-
-  for (int SrcIdx : SrcIndices) {
-    if (SrcIdx == -1) break;
-    const MCOperand &Src = Inst.getOperand(SrcIdx);
-    if (Src.isReg()) {
-      if (TRI->regsOverlap(Dst.getReg(), Src.getReg())) {
-        const unsigned SrcReg = mc2PseudoReg(Src.getReg());
-        Error(getRegLoc(SrcReg, Operands),
-          "destination must be different than all sources");
-        return false;
-      }
-    }
-  }
-
-  return true;
-}
-
 bool AMDGPUAsmParser::validateIntClampSupported(const MCInst &Inst) {
 
   const unsigned Opc = Inst.getOpcode();
   const MCInstrDesc &Desc = MII.get(Opc);
 
   if ((Desc.TSFlags & SIInstrFlags::IntClamp) != 0 && !hasIntClamp()) {
     int ClampIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::clamp);
     assert(ClampIdx != -1);
if (!validateSOPLiteral(Inst)) {
     return false;
   }
   if (!validateVOPLiteral(Inst, Operands)) {
     return false;
   }
   if (!validateConstantBusLimitations(Inst, Operands)) {
     return false;
   }
-  if (!validateEarlyClobberLimitations(Inst, Operands)) {
-    return false;
-  }
   if (!validateIntClampSupported(Inst)) {
     Error(getImmLoc(AMDGPUOperand::ImmTyClampSI, Operands),
       "integer clamping is not supported on this GPU");
     return false;
   }
   if (!validateOpSel(Inst)) {
     Error(getImmLoc(AMDGPUOperand::ImmTyOpSel, Operands),
       "invalid op_sel operand");

llvm/test/MC/AMDGPU/gfx10_err_pos.s

 // RUN: not llvm-mc -arch=amdgcn -mcpu=gfx1010 -mattr=+WavefrontSize32,-WavefrontSize64 %s 2>&1 | FileCheck %s --implicit-check-not=error: --strict-whitespace
 
 //==============================================================================
-// destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[0:1], v[1:2], v9, v[4:5]
-// CHECK: error: destination must be different than all sources
-// CHECK-NEXT:{{^}}v_mqsad_pk_u16_u8 v[0:1], v[1:2], v9, v[4:5]
-// CHECK-NEXT:{{^}}                          ^
-
-v_mqsad_pk_u16_u8 v[0:1], v[2:3], v0, v[4:5]
-// CHECK: error: destination must be different than all sources
-// CHECK-NEXT:{{^}}v_mqsad_pk_u16_u8 v[0:1], v[2:3], v0, v[4:5]
-// CHECK-NEXT:{{^}}                                  ^
-
-v_mqsad_pk_u16_u8 v[0:1], v[2:3], v1, v[4:5]
-// CHECK: error: destination must be different than all sources
-// CHECK-NEXT:{{^}}v_mqsad_pk_u16_u8 v[0:1], v[2:3], v1, v[4:5]
-// CHECK-NEXT:{{^}}                                  ^
-
-v_mqsad_pk_u16_u8 v[0:1], v[2:3], v9, v[0:1]
-// CHECK: error: destination must be different than all sources
-// CHECK-NEXT:{{^}}v_mqsad_pk_u16_u8 v[0:1], v[2:3], v9, v[0:1]
-// CHECK-NEXT:{{^}}                                      ^
-
-//==============================================================================
 // dim modifier is required on this GPU
 
 image_atomic_add v252, v2, s[8:15]
 // CHECK: error: dim modifier is required on this GPU
 // CHECK-NEXT:{{^}}image_atomic_add v252, v2, s[8:15]
 // CHECK-NEXT:{{^}}^
 
 //==============================================================================

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

 // RUN: not llvm-mc -arch=amdgcn %s 2>&1 | FileCheck %s --check-prefix=GFX67 --check-prefix=GCN --implicit-check-not=error:
 // RUN: not llvm-mc -arch=amdgcn -mcpu=tahiti %s 2>&1 | FileCheck %s --check-prefix=GFX67 --check-prefix=GCN --implicit-check-not=error:
 // RUN: not llvm-mc -arch=amdgcn -mcpu=fiji %s 2>&1 | FileCheck %s --check-prefix=GFX89 --check-prefix=GCN --implicit-check-not=error:
 // RUN: not llvm-mc -arch=amdgcn -mcpu=gfx900 %s 2>&1 | FileCheck %s --check-prefix=GFX89 --check-prefix=GCN --implicit-check-not=error:
 
 v_add_f32_e64 v0, v1
 // GCN: error: too few operands for instruction
 
 v_div_scale_f32  v24, vcc, v22, 1.1, v22
 // GCN: error: literal operands are not supported
 
 v_mqsad_u32_u8 v[0:3], s[2:3], v4, v[0:3]
 // GFX67: error: instruction not supported on this GPU
-// GFX89: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[0:1], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[1:2], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[2:3], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[3:4], v[0:1], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[4:5], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[5:6], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[8:9], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
-
-v_mqsad_pk_u16_u8 v[9:10], v[1:2], v9, v[4:5]
-// GCN: error: destination must be different than all sources
 
 v_cmp_eq_f32_e64 vcc, v0, v1 mul:2
 // GCN: error: invalid operand for instruction
 
 v_cmp_le_f64_e64 vcc, v0, v1 mul:4
 // GCN: error: invalid operand for instruction
 
 //