[PATCH] D129095: [AMDGPU][CodeGen] Match SMRDs with constant bases and register offsets.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll:45
+}
+
 declare void @llvm.amdgcn.raw.buffer.store.v4i32(<4 x i32>, <4 x i32>, i32, i32, i32 immarg) #1
----------------
kosarev wrote:
> arsenm wrote:
> > kosarev wrote:
> > > arsenm wrote:
> > > > kosarev wrote:
> > > > > arsenm wrote:
> > > > > > Doesn't test the 32-bit constant case
> > > > > Why would we want to check that? Looks like there's nothing specific to 32-bit constants in the change?
> > > > You have the call to Expand32BitAddress
> > > Yes, which replicates the existing and already-tested logic. Will it eliminate the need for the test if I move the couples of `SelectSMRDOffset()` and `Expand32BitAddress()` calls into a separate function, something like `SelectSMRDBaseOffset()`? We would need it for matching (register + immediate) offsets anyway.
> > > 
> > > Feels like that'd be a test case for what is actually not a special case.
> > It's another permutation of a somewhat fragile case (which also works differently in globalisel). There's no reason to skimp on tests
> The `if ((Addr.getValueType() != MVT::i32 || Addr->getFlags().hasNoUnsignedWrap()))` bit in `AMDGPUDAGToDAGISel::SelectSMRD()` means for such a test we would need the `nuw` flag be set on the `ISD::ADD` in the offset, which we only do for constant `getelementptr inbounds` indexes that in turn would always go to the RHS operand of the `ISD::ADD`, and thus can't help reproducing the case.
> 
> D15544 gives some reasoning for raising `nuw` for non-negative constant offsets. If we think it's worth it, I could try to do the same for non-constants known to be small enough, but it again would help if we do not require cultivating things specific to the constant address space be a prerequisite for this generic change.
You can just add noinbounds to the getelementptr. It's not a special change and just testing the mechanics for what you already have here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129095