[PATCH] D46992: [AMDGPU] Add perf hints to functions

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:36
+
+static cl::opt<float> MemBoundThresh("amdgpu-membound-thresh",
+  cl::init(50),
----------------
Naming convention for existing flags seem to all use the full word threshold (same for the rest)


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:159
+  DEBUG(dbgs() << "[isIndirectAccess] " << *Inst << '\n');
+  DenseSet<const Value *> WorkSet;
+  DenseSet<const Value *> Visited;
----------------
Seems like a SmallSet?


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:240-242
+        if(isIndirectAccess(&I))
+          ++FI.IAMInstCount;
+        if(isLargeStride(&I))
----------------
Run clang-format


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:293
+
+  AMDGPUPerfHintAnalysis::FuncInfoMap::iterator Loc = FIM.find(&F);
+  assert(Loc != FIM.end() && "No func info");
----------------
Move this up to avoid calling the same find twice? visit can also return the inserted iterator


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:315
+bool AMDGPUPerfHint::isMemBound(const AMDGPUPerfHintAnalysis::FuncInfo &FI) {
+  return static_cast<double>(FI.MemInstCount) / FI.InstCount * 100 >
+         MemBoundThresh;
----------------
I think there's a policy of generally avoiding FP computations


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.cpp:320
+bool AMDGPUPerfHint::needLimitWave(const AMDGPUPerfHintAnalysis::FuncInfo& FI) {
+  return static_cast<double>(FI.MemInstCount
+                             + FI.IAMInstCount * IAWeight
----------------
Ditto


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHintAnalysis.h:1
+//===- AMDGPUPerfHintAnalysis.cpp - analysis of functions memory traffic --===//
+//
----------------
c++ mode comment


================
Comment at: lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:182
+      MemoryBound = PHA->isMemoryBound(&MF.getFunction());
+      WaveLimiter= PHA->needsWaveLimiter(&MF.getFunction());
+    }
----------------
Missing space


https://reviews.llvm.org/D46992