[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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130263