[PATCH] D79288: [AMDGPU][MC] Enabled 21-bit signed offsets for SMEM instructions
Dmitry Preobrazhensky via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 4 15:05:22 PDT 2020
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
More information about the llvm-commits
mailing list