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

[hsmhsm]

Summary

Now since LDS uses within non-kernel functions are being handled in the
pass - LowerModuleLDS, we *NO* need to *forcefully* inline non-kernel
functions just because they use LDS. Do forceful inlining only when the
pass - LowerModuleLDS is not enabled. It is enabled by default.

[JonChesterfield]

I agree with the intent. This would be a good change to run through the internal CI before landing.

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

129

What's REGION_ADDRESS? LowerModuleLDS doesn't to do anything with it so this looks like an incidental behaviour change

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

396

Why does the cl::opt instance initialize to true, while this initializes to false?

I'm not clear why the storage needs to become external here, but do see that the same thing has been done for the similar variables in this block.

[hsmhsm]

In D100481#2689032, @JonChesterfield wrote:

I agree with the intent. This would be a good change to run through the internal CI before landing.

I agree to some extent that it will be good change to run through the internal CI before landing, but, not sure, why you did not take care of it while you were landing your original patch. Because, without this change, your patch is practically not much applicable.

Can we upstream it and watch out how your patch really performs with this change?

@arsenm what would be your suggestion here?

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

129

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.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

396

cl::opt instance initialize it to true, because, by default the flag is enabled, and also this same instance make sure that AMDGPUTargetMachine::EnableLowerModuleLDS is correctly initialized to default true. Please have a look at cl::location(AMDGPUTargetMachine::EnableLowerModuleLDS).

However, since AMDGPUTargetMachine::EnableLowerModuleLDS is a static variable, it is initialized to "false" at class scope.

The storage needs to be external, because it needs to used at different TU, like this one used within AMDGPUAlwaysInlinePass.cpp.

For example, please take a look at the flag EnableAMDGPUFunctionCallsOpt, and its corresponding static member - AMDGPUTargetMachine::EnableFunctionCalls.

I just followed the same philosophy here.

[JonChesterfield]

Change requested is to not change the handling of region_address. I haven't looked up what that is but I am sure the module lds pass doesn't do anything with it.

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

129

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.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

396

I would say you've copied a mistake from enable function calls. Look at EnableFixedFunctionABI for a case where the initializer is consistent between the two places.

It looks like none of these need explicit initializers to me, but if we're going to have one, lets have it match the default for the command line argument.

[hsmhsm]

Fixed review comments by Jon.

[JonChesterfield]

LG, thanks

[t-tye]

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

129

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 .

[JonChesterfield]

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

129

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.

[arsenm]

Graphics does use region, so we can't break that

llvm/lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp

static bool alwaysInlineImpl(Module &M, bool GlobalOpt) {
   //
   // OpenCL doesn't allow declaring LDS in non-kernels, so in practice this
   // should only appear when IPO passes manages to move LDs defined in a kernel
   // into a single user function.
 
   for (GlobalVariable &GV : M.globals()) {
     // TODO: Region address
     unsigned AS = GV.getAddressSpace();
-    if (AS != AMDGPUAS::LOCAL_ADDRESS && AS != AMDGPUAS::REGION_ADDRESS)
-      continue;
-
+    if ((AS == AMDGPUAS::REGION_ADDRESS) ||
+        (AS == AMDGPUAS::LOCAL_ADDRESS &&
+         !AMDGPUTargetMachine::EnableLowerModuleLDS))
       recursivelyVisitUsers(GV, FuncsToAlwaysInline);
   }
 
   if (!AMDGPUTargetMachine::EnableFunctionCalls || StressCalls) {
     auto IncompatAttr
       = StressCalls ? Attribute::AlwaysInline : Attribute::NoInline;
 
     for (Function &F : M) {
       if (!F.isDeclaration() && !F.use_empty() &&
           !F.hasFnAttribute(IncompatAttr)) {
         if (StressCalls) {
           if (!FuncsToAlwaysInline.count(&F))

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.h

protected:
 
   StringRef getGPUName(const Function &F) const;
   StringRef getFeatureString(const Function &F) const;
 
 public:
   static bool EnableLateStructurizeCFG;
   static bool EnableFunctionCalls;
   static bool EnableFixedFunctionABI;
+  static bool EnableLowerModuleLDS;
 
   AMDGPUTargetMachine(const Target &T, const Triple &TT, StringRef CPU,
                       StringRef FS, TargetOptions Options,
                       Optional<Reloc::Model> RM, Optional<CodeModel::Model> CM,
                       CodeGenOpt::Level OL);
   ~AMDGPUTargetMachine() override;
 
   const TargetSubtargetInfo *getSubtargetImpl() const;

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

static cl::opt<bool> EnableScalarIRPasses(
   cl::init(true),
   cl::Hidden);
 
 static cl::opt<bool> EnableStructurizerWorkarounds(
     "amdgpu-enable-structurizer-workarounds",
     cl::desc("Enable workarounds for the StructurizeCFG pass"), cl::init(true),
     cl::Hidden);
 
-static cl::opt<bool>
-    EnableLowerModuleLDS("amdgpu-enable-lower-module-lds", cl::Hidden,
-                         cl::desc("Enable lower module lds pass"),
-                         cl::init(true));
+static cl::opt<bool, true> EnableLowerModuleLDS(
+    "amdgpu-enable-lower-module-lds", cl::desc("Enable lower module lds pass"),
+    cl::location(AMDGPUTargetMachine::EnableLowerModuleLDS), cl::init(true),
+    cl::Hidden);
 
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAMDGPUTarget() {
   // Register the target
   RegisterTargetMachine<R600TargetMachine> X(getTheAMDGPUTarget());
   RegisterTargetMachine<GCNTargetMachine> Y(getTheGCNTarget());
 
   PassRegistry *PR = PassRegistry::getPassRegistry();
   initializeR600ClauseMergePassPass(*PR);
if (TT.getArch() == Triple::amdgcn) {
     else if (getMCSubtargetInfo()->checkFeatures("+wavefrontsize32"))
       MRI.reset(llvm::createGCNMCRegisterInfo(AMDGPUDwarfFlavour::Wave32));
   }
 }
 
 bool AMDGPUTargetMachine::EnableLateStructurizeCFG = false;
 bool AMDGPUTargetMachine::EnableFunctionCalls = false;
 bool AMDGPUTargetMachine::EnableFixedFunctionABI = false;
+bool AMDGPUTargetMachine::EnableLowerModuleLDS = true;
 
 AMDGPUTargetMachine::~AMDGPUTargetMachine() = default;
 
 StringRef AMDGPUTargetMachine::getGPUName(const Function &F) const {
   Attribute GPUAttr = F.getFnAttribute("target-cpu");
   return GPUAttr.isValid() ? GPUAttr.getValueAsString() : getTargetCPU();
 }
 

llvm/test/CodeGen/AMDGPU/force-alwaysinline-lds-global-address-codegen.ll

-; RUN: llc -mtriple=amdgcn-amd-amdhsa -amdgpu-function-calls -amdgpu-stress-function-calls < %s | FileCheck -check-prefix=GCN %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls < %s | FileCheck -check-prefix=GCN %s
-; RUN: llc -mtriple=amdgcn-amd-amdhsa < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -amdgpu-function-calls -amdgpu-stress-function-calls -amdgpu-enable-lower-module-lds=false < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls -amdgpu-enable-lower-module-lds=false < %s | FileCheck -check-prefix=GCN %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -amdgpu-enable-lower-module-lds=false < %s | FileCheck -check-prefix=GCN %s
 
 @lds0 = addrspace(3) global i32 undef, align 4
 
 ; GCN-NOT: load_lds_simple
 
 define internal i32 @load_lds_simple() {
   %load = load i32, i32 addrspace(3)* @lds0, align 4
   ret i32 %load

llvm/test/CodeGen/AMDGPU/force-alwaysinline-lds-global-address.ll

-; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-always-inline %s | FileCheck --check-prefix=ALL %s
-; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=amdgpu-always-inline %s | FileCheck --check-prefix=ALL %s
-; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls -amdgpu-always-inline %s | FileCheck --check-prefix=ALL %s
-; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls -passes=amdgpu-always-inline %s | FileCheck --check-prefix=ALL %s
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-always-inline -amdgpu-enable-lower-module-lds=false %s | FileCheck --check-prefix=ALL %s
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -passes=amdgpu-always-inline -amdgpu-enable-lower-module-lds=false %s | FileCheck --check-prefix=ALL %s
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls -amdgpu-always-inline -amdgpu-enable-lower-module-lds=false %s | FileCheck --check-prefix=ALL %s
+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-stress-function-calls -passes=amdgpu-always-inline -amdgpu-enable-lower-module-lds=false %s | FileCheck --check-prefix=ALL %s
 
 target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5"
 
 @lds0 = addrspace(3) global i32 undef, align 4
 @lds1 = addrspace(3) global [512 x i32] undef, align 4
 @nested.lds.address = addrspace(1) global i32 addrspace(3)* @lds0, align 4
 @gds0 = addrspace(2) global i32 undef, align 4