[PATCH] D128836: [AMDGPU][GlobalISel] Support register offsets for SMRDs.

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

LGTM with a tweak to inst-select-load-smrd.mir.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3225
 
   if (mi_match(Def->getOperand(2).getReg(), MRI, m_ZeroInt())) {
     return Def->getOperand(1).getReg();
----------------
Not your fault, but this seems to be missing checks (or asserts) on the number of operands and the type of the operands of the G_MERGE_VALUES.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3825
   // FIXME: We should shrink the GEP if the offset is known to be <= 32-bits,
-  // then we can select all ptr + 32-bit offsets not just immediate offsets.
-  if (AddrInfo.empty() || AddrInfo[0].SgprParts.size() != 1)
+  // then we can select all ptr + 32-bit offsets.
+  if (AddrInfo.empty())
----------------
I don't understand this comment. What cases are we still missing?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-load-smrd.mir:232
+
+# Test a load with a register offset.
+# GCN-LABEL: name: smrd_sgpr{{$}}
----------------
Please put these checks before the function, not after. (But wow the existing checks in this file are really messy.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128836