[PATCH] D79288: [AMDGPU][MC] Enabled 21-bit signed offsets for SMEM instructions

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp marked 2 inline comments as done.
dp added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp:369
+  const MCOperand &MO = MI.getOperand(OpNo);
+  assert(MO.isImm());
+
----------------
arsenm wrote:
> This assert is redundant, the getImm below will assert anyway
My intention was to state explicitly that an immediate is expected. But this is a matter of taste. I’ll remove the assert. 


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/SIMCCodeEmitter.cpp:372-373
+  auto Offset = MO.getImm();
+  if (AMDGPU::isVI(STI)) {
+    Offset &= 0xFFFFF;       // VI only supports 20-bit unsigned offsets.
+  }
----------------
arsenm wrote:
> Do we really need to explicitly clamp this here?
This is not strictly necessary. However AMD documentation states that unused bits in instruction encoding should be zero. An MC instruction may come directly from codegen, and I’m not sure it does SMEM offset validation. Should I replace this truncation with an assert?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79288/new/

https://reviews.llvm.org/D79288