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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3833
   // SGPR offset is unsigned.
-  if (!GEPInfo.Imm || GEPInfo.Imm < 0 || !isUInt<32>(GEPInfo.Imm))
-    return None;
+  if (AddrInfo[0].SgprParts.size() == 1 && GEPInfo.Imm > 0 &&
+      isUInt<32>(GEPInfo.Imm)) {
----------------
arsenm wrote:
> Isn't GEPInfo.Imm > 0 redundant with isUInt<32>?
I believe `isUInt<32>()` allows zero?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3848
+    Register OffsetReg = GEPInfo.SgprParts[1];
+    mi_match(OffsetReg, *MRI, m_GZExt(m_Reg(OffsetReg)));
+    if (MRI->getType(OffsetReg) == LLT::scalar(32)) {
----------------
arsenm wrote:
> Should use matchZeroExtendFromS32 to handle all the extended cases. Also this isn't checking the match return
Changed.

> Also this isn't checking the match return

Yes, intentionally. In the previous version, as long as the operand originates from a 32-bit scalar, we don't really care if zero-expansion matched. Though I couldn't provide a reproducer for the case without expansion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128836