[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/AMDGPUInstructionSelector.cpp:4886
+      AMDGPU::getBaseWithConstantOffset(*MRI, Root.getReg());
+  if (!SOffset || MRI->getType(SOffset) != LLT::scalar(32) || Offset == 0)
+    return None;
----------------
kosarev wrote:
> foad wrote:
> > Does anything go wrong if you remove the `Offset == 0` check?
> The tests still pass (and that is what I would expect from reading relevant code), but we don't have many of them and GISel doesn't seem to be able to cope with the most of our test corpus shaders. Is there any benefit in removing the check?
> Is there any benefit in removing the check?
I have not fully understood the patch but it seems like it is more complicated than necessary because you insist that this offset has to be non-zero. For example I assume this is why you had to move the `Offset && !SOffset` case out of SelectSMRDBaseOffset and into SelectSMRD(?). So I am trying to understand where the requirement comes from. Is it because you want to be sure that if the offset is 0 then we select _SGPR instead of _SGPR_IMM forms of the instruction? If so, then I think this should be handled by ensuring that the patterns for the _SGPR forms have higher priority. (Perhaps this already happens, just because of the order the patterns appear in the .td file?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130263