[PATCH] D94648: [amdgpu] Implement lower function LDS pass

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:515
+        if (PassName == "amdgpu-lower-module-lds") {
+          if (!DisableLowerModuleLDS) {
+            PM.addPass(AMDGPULowerModuleLDSPass());
----------------
JonChesterfield wrote:
> aeubanks wrote:
> > This should always be added here regardless of the flag, this is just for registering that the pass exists. Rather, the pass should also be added in `registerCGSCCOptimizerLateEPCallback()` below guarded by the flag.
> Welcome! Thank you very much for the input, there's been more guesswork in the new pass manager parts than I like. Can't seem to find any documentation on how it works. Dropped the test here, added to CGSCC. It's a module pass and the other things there were function passes, but instantiating a new ModulePassManager seems to work fine.
I should definitely write some documentation somewhere.

Adding it to a `ModulePassManager` you created doesn't do anything since it's not getting added to the overall pipeline. For example, the `FunctionPassManager` there is added to the `CGSCCPassManager`.
For the legacy PM it doesn't really make sense to add a module pass at `EP_CGSCCOptimizerLate` since it'll end up breaking the CGSCC pipeline. Normally it runs the the CGSCC passes and the function pipeline on each function in an SCC as it visits the SCCs, but with a module pass in the middle those will get split up. The new PM just makes this whole thing explicit via nesting when adding passes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94648