[AMDGPU][MC] Added validation of image dst/data size (must match dmask and tfe)

[dp]

Summary

See bug 36000: https://bugs.llvm.org/show_bug.cgi?id=36000

[dp]

Disabled validation of gather4 instructions because they have special rules not described in AMD documents.

Could anybody provide an up-to-date gather4 description?

[arsenm]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

2281 ↗

(On Diff #131284)

New line

2284 ↗

(On Diff #131284)

New line

2295 ↗

(On Diff #131284)

I've been thinking we should probably eventually remove the TFE operands from instructions. Since this changes the register class of the result, so it necessitates another set of instruction definitions

2297 ↗

(On Diff #131284)

New line

2307–2308 ↗

(On Diff #131284)

New line.

2308 ↗

(On Diff #131284)

Checking for atomic this way is a bit suspect but I think this will work

[dp]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

2295 ↗

(On Diff #131284)

I thought about adding TFE variants though the whole thing already looks ugly enough.
Also I'm thinking about excluding d16 from validation - looks like these variants have special (badly documented) rules.

2308 ↗

(On Diff #131284)

Should I add a new flag for atomics to identify them safely?

[dp]

Corrected typos found by Matt.
Excluded d16 from validation.

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

private:
 
   void errorExpTgt();
   OperandMatchResultTy parseExpTgtImpl(StringRef Str, uint8_t &Val);
 
   bool validateInstruction(const MCInst &Inst, const SMLoc &IDLoc);
   bool validateConstantBusLimitations(const MCInst &Inst);
   bool validateEarlyClobberLimitations(const MCInst &Inst);
   bool validateIntClampSupported(const MCInst &Inst);
+  bool validateMIMGAtomicDMask(const MCInst &Inst);
+  bool validateMIMGDataSize(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");
if ((Desc.TSFlags & SIInstrFlags::IntClamp) != 0 && !hasIntClamp()) {
     int ClampIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::clamp);
     assert(ClampIdx != -1);
     return Inst.getOperand(ClampIdx).getImm() == 0;
   }
 
   return true;
 }
 
+bool AMDGPUAsmParser::validateMIMGDataSize(const MCInst &Inst) {
+
+  const unsigned Opc = Inst.getOpcode();
+  const MCInstrDesc &Desc = MII.get(Opc);
+
+  if ((Desc.TSFlags & SIInstrFlags::MIMG) == 0)
+    return true;
+
+  // Gather4 instructions seem to have special rules not described in spec.
+  if (Desc.TSFlags & SIInstrFlags::Gather4)
+    return true;
+
+  int VDataIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::vdata);
+  int DMaskIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::dmask);
+  int TFEIdx   = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::tfe);
+
+  assert(VDataIdx != -1);
+  assert(DMaskIdx != -1);
+  assert(TFEIdx != -1);
+
+  unsigned VDataSize = AMDGPU::getRegOperandSize(getMRI(), Desc, VDataIdx);
+  unsigned TFESize = Inst.getOperand(TFEIdx).getImm()? 1 : 0;
+  unsigned DMask = Inst.getOperand(DMaskIdx).getImm() & 0xf;
+  if (DMask == 0)
+    DMask = 1;
+
+  return (VDataSize / 4) == countPopulation(DMask) + TFESize;
+}
+
+bool AMDGPUAsmParser::validateMIMGAtomicDMask(const MCInst &Inst) {
+
+  const unsigned Opc = Inst.getOpcode();
+  const MCInstrDesc &Desc = MII.get(Opc);
+
+  if ((Desc.TSFlags & SIInstrFlags::MIMG) == 0)
+    return true;
+  if (!Desc.mayLoad() || !Desc.mayStore())
+    return true; // Not atomic
+
+  int DMaskIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::dmask);
+  unsigned DMask = Inst.getOperand(DMaskIdx).getImm() & 0xf;
+
+  // This is an incomplete check because image_atomic_cmpswap
+  // may only use 0x3 and 0xf while other atomic operations
+  // may use 0x1 and 0x3. However these limitations are
+  // verified when we check that dmask matches dst size.
+  return DMask == 0x1 || DMask == 0x3 || DMask == 0xf;
+}
+
 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;
   }
   if (!validateIntClampSupported(Inst)) {
     Error(IDLoc,
       "integer clamping is not supported on this GPU");
     return false;
   }
+  if (!validateMIMGDataSize(Inst)) {
+    Error(IDLoc,
+      "image data size does not match dmask and tfe");
+    return false;
+  }
+  if (!validateMIMGAtomicDMask(Inst)) {
+    Error(IDLoc,
+      "invalid atomic image dmask");
+    return false;
+  }
 
   return true;
 }
 
 static std::string AMDGPUMnemonicSpellCheck(StringRef S, uint64_t FBS,
                                             unsigned VariantID = 0);
 
 bool AMDGPUAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,

llvm/trunk/test/MC/AMDGPU/mimg-err.s

+// RUN: not llvm-mc -arch=amdgcn -show-encoding %s 2>&1 | FileCheck %s --check-prefix=NOGCN
+// RUN: not llvm-mc -arch=amdgcn -mcpu=tahiti -show-encoding %s 2>&1 | FileCheck %s --check-prefix=NOGCN
+// RUN: not llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s 2>&1 | FileCheck %s --check-prefix=NOGCN
+
+//===----------------------------------------------------------------------===//
+// Image Load/Store
+//===----------------------------------------------------------------------===//
+
+image_load    v[4:6], v[237:240], s[28:35] dmask:0x7 tfe
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_load    v[4:5], v[237:240], s[28:35] dmask:0x7
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_store   v[4:7], v[237:240], s[28:35] dmask:0x7
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_store   v[4:7], v[237:240], s[28:35] dmask:0xe
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_load    v4, v[237:240], s[28:35] tfe
+// NOGCN: error: image data size does not match dmask and tfe
+
+//===----------------------------------------------------------------------===//
+// Image Sample
+//===----------------------------------------------------------------------===//
+
+image_sample  v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 tfe
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_sample  v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x3
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_sample  v[193:195], v[237:240], s[28:35], s[4:7] dmask:0xf
+// NOGCN: error: image data size does not match dmask and tfe
+
+//===----------------------------------------------------------------------===//
+// Image Atomics
+//===----------------------------------------------------------------------===//
+
+image_atomic_add v252, v2, s[8:15] dmask:0x1 tfe
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_atomic_add v[6:7], v255, s[8:15] dmask:0x2
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_atomic_add v[6:7], v255, s[8:15] dmask:0xf
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_atomic_cmpswap v[4:7], v[192:195], s[28:35] dmask:0xf tfe
+// NOGCN: error: image data size does not match dmask and tfe
+
+image_atomic_add v252, v2, s[8:15]
+// NOGCN: error: invalid atomic image dmask
+
+image_atomic_add v[6:7], v255, s[8:15] dmask:0x2 tfe
+// NOGCN: error: invalid atomic image dmask
+
+image_atomic_cmpswap v[4:7], v[192:195], s[28:35] dmask:0xe tfe
+// NOGCN: error: invalid atomic image dmask

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

 // RUN: llvm-mc -arch=amdgcn -show-encoding %s | FileCheck %s --check-prefix=SICI
 // RUN: llvm-mc -arch=amdgcn -mcpu=tahiti -show-encoding %s | FileCheck %s --check-prefix=SICI
 // RUN: llvm-mc -arch=amdgcn -mcpu=fiji -show-encoding %s | FileCheck %s --check-prefix=VI
 
 //===----------------------------------------------------------------------===//
 // Image Load/Store
 //===----------------------------------------------------------------------===//
 
 image_load    v[4:6], v[237:240], s[28:35] dmask:0x7 unorm
 // SICI: image_load v[4:6], v[237:240], s[28:35] dmask:0x7 unorm ; encoding: [0x00,0x17,0x00,0xf0,0xed,0x04,0x07,0x00]
 // VI:   image_load v[4:6], v[237:240], s[28:35] dmask:0x7 unorm ; encoding: [0x00,0x17,0x00,0xf0,0xed,0x04,0x07,0x00]
 
+image_load    v4, v[237:240], s[28:35]
+// SICI: image_load v4, v[237:240], s[28:35] ; encoding: [0x00,0x00,0x00,0xf0,0xed,0x04,0x07,0x00]
+// VI:   image_load v4, v[237:240], s[28:35] ; encoding: [0x00,0x00,0x00,0xf0,0xed,0x04,0x07,0x00]
+
+image_load    v[4:7], v[237:240], s[28:35] dmask:0x7 tfe
+// SICI: image_load v[4:7], v[237:240], s[28:35] dmask:0x7 tfe ; encoding: [0x00,0x07,0x01,0xf0,0xed,0x04,0x07,0x00]
+// VI:   image_load v[4:7], v[237:240], s[28:35] dmask:0x7 tfe ; encoding: [0x00,0x07,0x01,0xf0,0xed,0x04,0x07,0x00]
+
 image_store   v[193:195], v[237:240], s[28:35] dmask:0x7 unorm
 // SICI: image_store v[193:195], v[237:240], s[28:35] dmask:0x7 unorm ; encoding: [0x00,0x17,0x20,0xf0,0xed,0xc1,0x07,0x00]
 // VI:   image_store v[193:195], v[237:240], s[28:35] dmask:0x7 unorm ; encoding: [0x00,0x17,0x20,0xf0,0xed,0xc1,0x07,0x00]
 
+image_store   v193, v[237:240], s[28:35]
+// SICI: image_store v193, v[237:240], s[28:35] ; encoding: [0x00,0x00,0x20,0xf0,0xed,0xc1,0x07,0x00]
+// VI:   image_store v193, v[237:240], s[28:35] ; encoding: [0x00,0x00,0x20,0xf0,0xed,0xc1,0x07,0x00]
+
+image_store   v[193:194], v[237:240], s[28:35] tfe
+// SICI: image_store v[193:194], v[237:240], s[28:35] tfe ; encoding: [0x00,0x00,0x21,0xf0,0xed,0xc1,0x07,0x00]
+// VI:   image_store v[193:194], v[237:240], s[28:35] tfe ; encoding: [0x00,0x00,0x21,0xf0,0xed,0xc1,0x07,0x00]
+
 //===----------------------------------------------------------------------===//
 // Image Sample
 //===----------------------------------------------------------------------===//
 
 image_sample  v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 unorm
 // SICI: image_sample v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 unorm ; encoding: [0x00,0x17,0x80,0xf0,0xed,0xc1,0x27,0x00]
 // VI:   image_sample v[193:195], v[237:240], s[28:35], s[4:7] dmask:0x7 unorm ; encoding: [0x00,0x17,0x80,0xf0,0xed,0xc1,0x27,0x00]
 
+image_sample  v193, v[237:240], s[28:35], s[4:7]
+// SICI: image_sample v193, v[237:240], s[28:35], s[4:7] ; encoding: [0x00,0x00,0x80,0xf0,0xed,0xc1,0x27,0x00]
+// VI:   image_sample v193, v[237:240], s[28:35], s[4:7] ; encoding: [0x00,0x00,0x80,0xf0,0xed,0xc1,0x27,0x00]
+
+image_sample  v[193:194], v[237:240], s[28:35], s[4:7] tfe
+// SICI: image_sample v[193:194], v[237:240], s[28:35], s[4:7] tfe ; encoding: [0x00,0x00,0x81,0xf0,0xed,0xc1,0x27,0x00]
+// VI:   image_sample v[193:194], v[237:240], s[28:35], s[4:7] tfe ; encoding: [0x00,0x00,0x81,0xf0,0xed,0xc1,0x27,0x00]
+
 //===----------------------------------------------------------------------===//
 // Image Atomics
 //===----------------------------------------------------------------------===//
 
 image_atomic_add v4, v[192:195], s[28:35] dmask:0x1 unorm glc
 // SICI: image_atomic_add v4, v[192:195], s[28:35] dmask:0x1 unorm glc ; encoding: [0x00,0x31,0x44,0xf0,0xc0,0x04,0x07,0x00]
 // VI:   image_atomic_add v4, v[192:195], s[28:35] dmask:0x1 unorm glc ; encoding: [0x00,0x31,0x48,0xf0,0xc0,0x04,0x07,0x00]