[PATCH] D129381: [AMDGPU][CodeGen] Support (register + immediate) SMRD offsets.

Mon Jul 11 10:38:06 PDT 2022

arsenm added a comment.

In D129381#3642390 <https://reviews.llvm.org/D129381#3642390>, @kosarev wrote:

> In D129381#3639233 <https://reviews.llvm.org/D129381#3639233>, @kosarev wrote:
>
>> I'm looking into what's going on there.
>
> The reason seems to be that with this change in place we generate more spills, which in turn is caused by reduced number of used registers as determined by `AMDGPUResourceUsageAnalysis::analyzeResourceUsage()` -- 102 registers originally vs 96 with the change. I'm not sure how precise the analysis is expected to be and whether this needs a ticket. Would appreciate any advice on this.

That should be precise except in the presence of indirect/external/unanalyzeable calls. I'm a bit surprised there would be any big difference from this, but this is certainly going to be from secondary effects

================
Comment at: llvm/test/CodeGen/AMDGPU/amdgcn-load-offset-from-reg.ll:1-2
-; RUN: llc -march=amdgcn -global-isel=0 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,SDAG %s
-; RUN: llc -march=amdgcn -global-isel=1 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,GISEL %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -global-isel=0 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,SDAG %s
+; RUN: llc -march=amdgcn -mcpu=gfx900 -global-isel=1 -verify-machineinstrs -stop-after=amdgpu-isel -o - %s | FileCheck -check-prefixes=GCN,GISEL %s

----------------
kosarev wrote:
> kosarev wrote:
> > arsenm wrote:
> > > You shouldn't use -stop-after=amdgpu-isel. Use finalize-isel for MIR tests (although I'm not sure why you're testing MIR here)
> > This seems to be where we already have checks for a similar case? Is there a better place for that?
> > You shouldn't use -stop-after=amdgpu-isel.
> 
> Can you expand a bit more on why this is preferable, please? I see literally 4 tests utilising finalize-isel, none of them look like purely isel tests,  and the other 35 all seem to use amdgpu-isel. The description of finalize-isel as it comes from rG9cac4e6d14035 again doesn't suggest anything obviously useful for this test?
> 
amdgpu-isel is specifically the SDAG pass. Your GISEL check didn't stop anywhere near the same place, and -stop-pass ran off the end and completed register allocation

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129381