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

[Ivan Kosarev via Phabricator via llvm-commits]

kosarev added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:40
+      unsigned OtherOp = 3 - Op;
+      if (mi_match(Def->getOperand(Op).getReg(), MRI, m_ICst(Offset)))
+        return std::make_pair(Def->getOperand(OtherOp).getReg(), Offset);
----------------
arsenm wrote:
> kosarev wrote:
> > arsenm wrote:
> > > kosarev wrote:
> > > > arsenm wrote:
> > > > > kosarev wrote:
> > > > > > I'm not perfectly sure we do the right here. One concern is that `m_ICst()` seems to match signed values and another is that we ignore `nuw`/`nsw` flags. I tried to add a check for `nuw`, and that led to numerous test failures, which coupled with that we seem to use this code for non-scalar buffer loads suggests that it's probably a task of its own. If anyone can confirm that these concerns are valid, I'd like to add a TODO explaining them to this patch.
> > > > > m_ICst should just match constants, the sign is in the interpretation.
> > > > > 
> > > > > nuw/nsw only potentially matters in cases where we're extending a 32-bit offset to 64-bits in one of the offsets that don't handle wrapping. I expect to match the DAG behavior for all of these addressing modes
> > > > > 
> > > > > I don't think you should be trying to match constant offsets in either operand here. This is showing we're missing the standard canonicalization to move constants to the RHS for gMIR
> > > > It seems the current approach is to rely on matchers that for commutative operations try both the cases. Updated to use them.
> > > It's not a current approach, it's a missing feature. I'd rather not handle the commuted case and separately introduce a new combine to canonicalize constants to the RHS
> > I don't mind to look into why we currently do it the way we do, but now that this patch doesn't do anything special to match both the cases, it seems it can be addressed separately and after this is landed?
> > 
> > (And as I side note, I admit I struggle to see how this is not a current approach, considering that this is literally what's being done whenever we aim to match commutative nodes.)
> I don't know why I thought this was checking both operands; I see now this is looking through a copy for a constant. This is still also a pattern that should not reach the selector, and if it does it's demonstrating a combiner failure. The common case where we end up with a copy of a constant is for a VGPR use, which you don't have here so I don't think you need to worry about the copy here
This patch doesn't add the copy matching either; just rewrites it to use `mi_match()`. Whether it's a combiner failure or a matcher issue as your FIXME above says, it doesn't seem to have anything to do with this patch?

Removing the copy-matching bit leads to some test failures, so looks like it's there for a reason.
```
Failed Tests (5):
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.gws.barrier.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ds.gws.init.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.s.buffer.load.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.mir
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130263