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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
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.
----------------
arsenm wrote:
> Comment needs update.
> 
> Maybe add a todo that this should be a machine analysis?
I do not think it has to be machine IR pass. It will be really difficult to perform this analysis on machine IR.


================
Comment at: lib/Target/AMDGPU/AMDGPUPerfHint.cpp:289
+  }
+  FIM[&F] = FI;
+}
----------------
arsenm wrote:
> 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
Only as opt test. BE will fail on recursion.


================
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());
+    }
+  }
----------------
arsenm wrote:
> 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?
By the time it is needed function's IR is already destroyed. Note, it is not only needed from printer, it is also checked in the scheduler.


https://reviews.llvm.org/D46992