[PATCH] D130263: [AMDGPU][CodeGen] Support (soffset + offset) s_buffer_load's.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1886
 
 // Match an immediate (if Imm is true) or an SGPR (if Imm is false)
 // offset. If Imm32Only is true, match only 32-bit immediate offsets
----------------
Would you mind updating this comment since there is no `Imm` argument.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1973
 
 // Match a base and an immediate (if Imm is true) or an SGPR
 // (if Imm is false) offset. If Imm32Only is true, match only 32-bit
----------------
Would you mind updating this comment since there is no `Imm` argument.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1989
   // wraparound, because s_load instructions perform the addition in 64 bits.
-  if ((Addr.getValueType() != MVT::i32 ||
-       Addr->getFlags().hasNoUnsignedWrap())) {
-    SDValue N0, N1;
-    // Extract the base and offset if possible.
-    if (CurDAG->isBaseWithConstantOffset(Addr) ||
-        Addr.getOpcode() == ISD::ADD) {
-      N0 = Addr.getOperand(0);
-      N1 = Addr.getOperand(1);
-    } else if (getBaseWithOffsetUsingSplitOR(*CurDAG, Addr, N0, N1)) {
-      assert(N0 && N1 && isa<ConstantSDNode>(N1));
-    }
-    if (N0 && N1) {
-      if (SelectSMRDOffset(N0, N1, SOffset, Offset, Imm32Only)) {
-        SBase = N0;
-        return true;
-      }
-      if (SelectSMRDOffset(N1, N0, SOffset, Offset, Imm32Only)) {
-        SBase = N1;
-        return true;
-      }
-    }
+  if (Addr.getValueType() == MVT::i32 && !Addr->getFlags().hasNoUnsignedWrap())
     return false;
----------------
Committing most of this hunk as an NFC refactoring would have made the current patch easier to read.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:4886
+      AMDGPU::getBaseWithConstantOffset(*MRI, Root.getReg());
+  if (!SOffset || MRI->getType(SOffset) != LLT::scalar(32))
+    return None;
----------------
It feels like the getType check could be an assertion? I can't see how it would fail, if the tablegen patterns are written correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130263