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

Fri Jul 15 10:51:39 PDT 2022

abinavpp added a comment.

In D129759#3652782 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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?

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:201
+// 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);
----------------
arsenm wrote:
> Maybe this should be inverted to be if the value is live out?
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129759