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

Sat Jul 16 08:31:56 PDT 2022

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.

================
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:
> > > kosarev wrote:
> > > > arsenm wrote:
> > > > > 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
> > > > Sorry, I'm not sure I follow. What is noinbounds and how it should be used with the getelementptr, exactly?
> > > @arsenm Ping?
> > You can try this:
> > ```
> > %i3 = getelementptr inbounds [4 x <2 x float>],
> > ```
> > 
> > with the addrspace(6) pointer
> It has already been explained in https://reviews.llvm.org/D129095#inline-1241075 that that wouldn't work.
> 
> On top of that, we don't seem to support addrspace(6) constants in SDAG ISel currently; I get AMDGPU DAG->DAG Pattern Instruction Selection error whenever I try to use one.
> 
> Regarding noinbounds, I don't see how your words explain what it is or how it can be useful for triggering the added code. Is that your comment still relevant?
> 
> And just for the record, I don't understand the argument about the new Expand32BitAddress() call being a sufficient reason for another test case. We usually don't add tests for higher level code just because it employs more of some lower level functions. It's up to tests specifically for these lower level functions to make sure they can handle their intended uses cases, and as hopefully is obvious from https://reviews.llvm.org/D129381 that eliminates multiple calls to Expand32BitAddress(), this patch doesn't add any new use cases for that function.
> 
> What //has// been changed is that we now support another permutation of operands in SelectSMRD(), but that is already covered with the added tests. addrspace(6) operands are no special in this regard, so adding more tests for them would mean we try to test combinations that are not special implementation-wise, which is again not how we write regression tests. And yes, unnecessary tests can actually be harmful -- they draw maintenance resources and attention and they hide cases that are really important. I believe some of our MC tests illustrate that.
> 
> Overall, I feel somewhat confused about your two last comments. If you can reword what you have to suggest in a bit more explicit way, that will likely be a great help. Thanks.
The point isn't to test Expand32BitAddress but to test that it's used on this path. I guess if constants don't work for constant 32-bit that's a separate issue

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129095