[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

New line

2284

New line

2295

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

New line

2307–2308

New line.

2308

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

[dp]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

2295

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

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

[dp]

Corrected typos found by Matt.
Excluded d16 from validation.

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;
+
+  // D16 variants seem to have special (poorly described) rules.
+  if (Desc.TSFlags & SIInstrFlags::D16)
+    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,

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
+

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]