[AMDGPU] Set amdgpu-memory-bound if a basic block has dense global memory access

[abinavpp]

Summary

AMDGPUPerfHintAnalysis doesn't set the memory bound attribute if
FuncInfo::InstCost outweighs MemInstCost even if we have a basic block
with relatively high global memory access. GCNSchedStrategy could revert
optimal scheduling in favour of occupancy which seems to degrade
performance for some kernels. This change introduces the
HasDenseGlobalMemAcc metric in the heuristic that makes the analysis
more conservative in these cases.

This fixes SWDEV-334259/SWDEV-343932

[rampitec]

This looks generally reasonable to me. Although I am thinking about a potential huge kernel with only a single block like this...
I guess it comes down to the question if that block is hot or cold. There is no perfect heuristic out there, but chances are higher a block is hot if it is in a loop.
Is the actual block you are considering in the regressed app inside a loop? Is that a "small single bb loop", i.e. when the bb branches right to itself? This is more likely to be a "dense memory access code" (and you will not need LoopInfo).

[arsenm]

I still think this pass should be replaced with a machine IR analysis

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

201

Maybe this should be inverted to be if the value is live out?

331

Weird use of ternary operator to use true here

[abinavpp]

In D129759#3652782, @rampitec wrote:

I guess it comes down to the question if that block is hot or cold. There is no perfect heuristic out there, but chances are higher a block is hot if it is in a loop.

I agree that we should try to guess how frequent the block with dense global
memory access would be executed.

In D129759#3652782, @rampitec wrote:

Is the actual block you are considering in the regressed app inside a loop? Is that a "small single bb loop", i.e. when the bb branches right to itself? This is more likely to be a "dense memory access code" (and you will not need LoopInfo).

Yes, the block is in a loop in the regressed kernel, but unfortunately it
doesn't branch to itself. So I guess we need LoopInfo. Unfortunately, I don't
think there's any way to get the LoopInfo analysis from a CallGraphSCCPass in
the legacy pass manager. I think we could use the
FunctionAnalysisManagerCGSCCProxy in the new pass manager
(https://llvm.org/docs/NewPassManager.html#using-analyses). Do you see any
problem in adding the new pass manager here? Or did I miss any other way to do
this in the legacy pass manager for now?

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

201

Did you mean to rename this in terms of live out, or return false if there's a
use of I outside its block? I'm not sure if the live out is relevant here.

[rampitec]

In D129759#3655803, @abinavpp wrote:

In D129759#3652782, @rampitec wrote:

Is the actual block you are considering in the regressed app inside a loop? Is that a "small single bb loop", i.e. when the bb branches right to itself? This is more likely to be a "dense memory access code" (and you will not need LoopInfo).

Yes, the block is in a loop in the regressed kernel, but unfortunately it
doesn't branch to itself. So I guess we need LoopInfo. Unfortunately, I don't
think there's any way to get the LoopInfo analysis from a CallGraphSCCPass in
the legacy pass manager. I think we could use the
FunctionAnalysisManagerCGSCCProxy in the new pass manager
(https://llvm.org/docs/NewPassManager.html#using-analyses). Do you see any
problem in adding the new pass manager here? Or did I miss any other way to do
this in the legacy pass manager for now?

No, I do not like to bring a new analysis here and a new PM either. Maybe we should try this as is.

[rampitec]

Let's see what will it bring and tune more when/if needed.

[abinavpp]

• Rebase
• Don't compute GlobalMemAccPercentage if HasDenseGlobalMemAcc is already set for the function
• Replace ternary expression with if

[abinavpp]

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

201

Are we good with this?

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

201

I mean invert the logic to see if the only users are outside the block. It doesn't really matter

[abinavpp]

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

201

We need to check for the user in the block regardless of the presence of users outside it, so I guess we can keep it this way.

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.h

public:
 
   bool needsWaveLimiter(const Function *F) const;
 
   struct FuncInfo {
     unsigned MemInstCost;
     unsigned InstCost;
     unsigned IAMInstCost; // Indirect access memory instruction count
     unsigned LSMInstCost; // Large stride memory instruction count
-    FuncInfo() : MemInstCost(0), InstCost(0), IAMInstCost(0), LSMInstCost(0) {}
+    bool HasDenseGlobalMemAcc; // Set if at least 1 basic block has relatively
+                               // high global memory access
+    FuncInfo()
+        : MemInstCost(0), InstCost(0), IAMInstCost(0), LSMInstCost(0),
+          HasDenseGlobalMemAcc(false) {}
   };
 
   typedef ValueMap<const Function*, FuncInfo> FuncInfoMap;
 
 private:
 
   FuncInfoMap FIM;
 };
 } // namespace llvm
 #endif // LLVM_LIB_TARGET_AMDGPU_MDGPUPERFHINTANALYSIS_H

llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp

#endif
   /// y = a[i+1000];
   /// z = a[i+2000];
   /// In the above example, the second and third memory access will be marked
   /// large stride memory access.
   bool isLargeStride(const Instruction *Inst);
 
   bool isGlobalAddr(const Value *V) const;
   bool isLocalAddr(const Value *V) const;
+  bool isGlobalLoadUsedInBB(const Instruction &) const;
 };
 
 static std::pair<const Value *, const Type *> getMemoryInstrPtrAndType(
     const Instruction *Inst) {
   if (auto LI = dyn_cast<LoadInst>(Inst))
     return {LI->getPointerOperand(), LI->getType()};
   if (auto SI = dyn_cast<StoreInst>(Inst))
     return {SI->getPointerOperand(), SI->getValueOperand()->getType()};
while (!WorkSet.empty()) {
 
     LLVM_DEBUG(dbgs() << "    dropped\n");
   }
 
   LLVM_DEBUG(dbgs() << "  is not IA\n");
   return false;
 }
 
+// Returns true if the global load `I` is used in its own basic block.
+bool AMDGPUPerfHint::isGlobalLoadUsedInBB(const Instruction &I) const {
+  const auto *Ld = dyn_cast<LoadInst>(&I);
+  if (!Ld)
+    return false;
+  if (!isGlobalAddr(Ld->getPointerOperand()))
+    return false;
+
+  for (const User *Usr : Ld->users()) {
+    if (const Instruction *UsrInst = dyn_cast<Instruction>(Usr)) {
+      if (UsrInst->getParent() == I.getParent())
+        return true;
+    }
+  }
+
+  return false;
+}
+
 AMDGPUPerfHintAnalysis::FuncInfo *AMDGPUPerfHint::visit(const Function &F) {
   AMDGPUPerfHintAnalysis::FuncInfo &FI = FIM[&F];
 
   LLVM_DEBUG(dbgs() << "[AMDGPUPerfHint] process " << F.getName() << '\n');
 
   for (auto &B : F) {
     LastAccess = MemAccessInfo();
+    unsigned UsedGlobalLoadsInBB = 0;
     for (auto &I : B) {
       if (const Type *Ty = getMemoryInstrPtrAndType(&I).second) {
         unsigned Size = divideCeil(Ty->getPrimitiveSizeInBits(), 32);
+        // TODO: Check if the global load and its user are close to each other
+        // instead (Or do this analysis in GCNSchedStrategy?).
+        if (isGlobalLoadUsedInBB(I))
+          UsedGlobalLoadsInBB += Size;
         if (isIndirectAccess(&I))
           FI.IAMInstCost += Size;
         if (isLargeStride(&I))
           FI.LSMInstCost += Size;
         FI.MemInstCost += Size;
         FI.InstCost += Size;
         continue;
       }
for (auto &I : B) {
                                        GEP->getPointerAddressSpace()))
           // Offset will likely be folded into load or store
           continue;
         ++FI.InstCost;
       } else {
         ++FI.InstCost;
       }
     }
+
+    if (!FI.HasDenseGlobalMemAcc) {
+      unsigned GlobalMemAccPercentage = UsedGlobalLoadsInBB * 100 / B.size();
+      if (GlobalMemAccPercentage > 50) {
+        LLVM_DEBUG(dbgs() << "[HasDenseGlobalMemAcc] Set to true since "
+                          << B.getName() << " has " << GlobalMemAccPercentage
+                          << "% global memory access\n");
+        FI.HasDenseGlobalMemAcc = true;
+      }
+    }
   }
 
   return &FI;
 }
 
 bool AMDGPUPerfHint::runOnFunction(Function &F) {
   const Module &M = *F.getParent();
   DL = &M.getDataLayout();
if (AMDGPU::isEntryFunctionCC(F.getCallingConv()) && needLimitWave(*Info)) {
     F.addFnAttr("amdgpu-wave-limiter", "true");
     Changed = true;
   }
 
   return Changed;
 }
 
 bool AMDGPUPerfHint::isMemBound(const AMDGPUPerfHintAnalysis::FuncInfo &FI) {
+  // Reverting optimal scheduling in favour of occupancy with basic block(s)
+  // having dense global memory access can potentially hurt performance.
+  if (FI.HasDenseGlobalMemAcc)
+    return true;
+
   return FI.MemInstCost * 100 / FI.InstCost > MemBoundThresh;
 }
 
 bool AMDGPUPerfHint::needLimitWave(const AMDGPUPerfHintAnalysis::FuncInfo &FI) {
   return ((FI.MemInstCost + FI.IAMInstCost * IAWeight +
            FI.LSMInstCost * LSWeight) * 100 / FI.InstCost) > LimitWaveThresh;
 }
 
 bool AMDGPUPerfHint::isGlobalAddr(const Value *V) const {
   if (auto PT = dyn_cast<PointerType>(V->getType())) {
     unsigned As = PT->getAddressSpace();
     // Flat likely points to global too.

llvm/test/CodeGen/AMDGPU/perfhint.ll

bb:
   %tmp7 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(1)* %arg, i64 %tmp6
   %tmp8 = load <4 x i32>, <4 x i32> addrspace(1)* %tmp7, align 16
   %tmp9 = getelementptr inbounds <4 x i32>, <4 x i32> addrspace(1)* %arg1, i64 %tmp6
   store <4 x i32> %tmp8, <4 x i32> addrspace(1)* %tmp9, align 16
   ret void
 }
 
 ; GCN-LABEL: {{^}}test_membound_1:
-; GCN: MemoryBound: 0
+; GCN: MemoryBound: 1
 define amdgpu_kernel void @test_membound_1(<2 x double> addrspace(1)* nocapture readonly %ptr.0,
                                            <2 x double> addrspace(1)* nocapture %ptr.1,
                                            <2 x double> %arg.0, i32 %arg.1, <4 x double> %arg.2) {
 bb.entry:
   %id.32 = tail call i32 @llvm.amdgcn.workitem.id.x()
   %id.0 = zext i32 %id.32 to i64
   %gep.0 = getelementptr inbounds <2 x double>, <2 x double> addrspace(1)* %ptr.0, i64 %id.0
   %ld.0 = load <2 x double>, <2 x double> addrspace(1)* %gep.0, align 16