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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:1
+//===-- AMDGPUPerfHint.cpp - Attach performance hints to functions --------===//
+//
----------------
Update comment


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:11-13
+/// \brief Adds amdgpu-hint-memory-bound metadata on a potentially memory bound
+/// functions and amdgpu-hint-wave-limiter on kernels which may benefit from
+/// limiting number of waves to reduce cache conflicts.
----------------
Comment needs update.

Maybe add a todo that this should be a machine analysis?


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:166
+
+bool AMDGPUPerfHint::isIndirectAccess(const Instruction *Inst) const {
+  DEBUG(dbgs() << "[isIndirectAccess] " << *Inst << '\n');
----------------
What does this mean exactly by indirect access?

This seems to me like it's reimplementing something like GetUnderlyingObject


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:259
+        ++FI.InstCount;
+      } else if (CallInst *CI = dyn_cast<CallInst>(&I)) {
+        Function *Callee = CI->getCalledFunction();
----------------
Probably should check for CallSite to cover the possible future case of InvokeInsts


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:260-261
+      } else if (CallInst *CI = dyn_cast<CallInst>(&I)) {
+        Function *Callee = CI->getCalledFunction();
+        if (Callee == NULL  || Callee->isDeclaration()) {
+          ++FI.InstCount;
----------------
!Callee


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:271
+        FuncInfoMap::iterator Loc = FIM.find(Callee);
+        assert (Loc != FIM.end() && "No func info");
+        FI.MemInstCount += Loc->second.MemInstCount;
----------------
Extra space


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:277-278
+      } else if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(&I)) {
+        APInt Off(DL->getIndexSizeInBits(GEP->getPointerAddressSpace()), 0);
+        if (GEP->accumulateConstantOffset(*DL, Off)) {
+          if (Off.isIntN(12))
----------------
isLegalAddressingMode (although at this point this should probably be a machine pass, but I understand that's more work to rewrite)


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:289
+  }
+  FIM[&F] = FI;
+}
----------------
rampitec wrote:
> t-tye wrote:
> > Should this be done at the beginning of the visit to ensure will terminate for mutual recursive functions?
> Thank you!
Can you add a test for this case


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:397
+  if (auto PT = dyn_cast<PointerType>(V->getType()))
+    return PT->getAddressSpace() == AS.CONSTANT_ADDRESS;
+  return false;
----------------
There's also CONSTANT_ADDRESS_32BIT


================
Comment at: lib/Target/AMDGPU/SIDefines.h:538-539
+
+#define ATTR_MEMBOUND_HINT       "amdgpu-hint-memory-bound"
+#define ATTR_WAVE_LIMITER_HINT   "amdgpu-hint-wave-limiter"
 } // End namespace llvm
----------------
Should be able to also remove this


================
Comment at: lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:178-184
+  if (auto Resolver = MF.getMMI().getResolver()) {
+    if (AMDGPUPerfHintAnalysis *PHA = static_cast<AMDGPUPerfHintAnalysis*>(
+          Resolver->getAnalysisIfAvailable(&AMDGPUPerfHintAnalysisID, true))) {
+      MemoryBound = PHA->isMemoryBound(&MF.getFunction());
+      WaveLimiter= PHA->needsWaveLimiter(&MF.getFunction());
+    }
+  }
----------------
It seems like there's no reason to actually put this code in SIMachineFunctionInfo. Can you just do this directly in the AsmPrinter where you emit this?


https://reviews.llvm.org/D46992