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

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 21 06:19:47 PDT 2022


arsenm 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);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130263



More information about the llvm-commits mailing list