[PATCH] D100481: [AMDGPU] Disable forceful inline of non-kernel functions which use LDS.

Wed Apr 14 14:24:10 PDT 2021

JonChesterfield added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp:125
+
+  if (!AMDGPUTargetMachine::EnableLowerModuleLDS) {
+    for (GlobalVariable &GV : M.globals()) {
----------------
t-tye wrote:
> JonChesterfield wrote:
> > hsmhsm wrote:
> > > JonChesterfield wrote:
> > > > What's REGION_ADDRESS? LowerModuleLDS doesn't to do anything with it so this looks like an incidental behaviour change
> > > Just like LOCAL_ADDRESS is specific to work-group,  REGION_ADDRESS is specific to device, but it is not yet implemented in AMDHSA.  Please have a look at the link - https://llvm.org/docs/AMDGPUUsage.html#address-spaces.
> > > 
> > > DS instructions can be used to access region memory too in addition to local memory. And, I guess, this is the region that code for local memory and region memory co-exist together at some places.
> > > 
> > > @arsenm might give better answer here about if it will result in an incidental behaviour change.
> > Let's refactor this so that it doesn't change behaviour for REGION_ADDRESS, as otherwise we're introducing at least one of confusion or a latent bug.
> REGION maps to the AMD GPU GDS (Global Data Store) scratchpad memory hardware. It is basically like LDS (Local Data Store) and uses the same ds instructions, but is shared by all waves on the same device.
> 
> The ROCm runtime has not implemented it. An earlier LLVM fork did have some support I believe. There are numerous places in AMD GPU LLVM that do have support for GDS such as in the SIMemoryLegalizer. It is described at https://llvm.org/docs/AMDGPUUsage.html#address-spaces .
Thanks! I know the term GDS but hadn't guessed the connection to REGION. It is indeed in the address spaces table.

Good to know that's what it referred to, the above is still correct. We do not want to change the handling of GDS based on whether a pass that only changes LDS has run.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100481