[PATCH] D119013: [ArgPromotion][AMDGPU] New MSSA-based function argument promotion pass with input/output argument support

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin marked 11 inline comments as done.
vpykhtin added a comment.

> Can you check if the testcases in llvm/test/CodeGen/AMDGPU/rewrite-out-arguments.ll are redundant and/or covered by the ones you add here?

@arsenm I looked into the test and its not directly suitable for this pass as behaives differently, however the cases can be used for testing.

> Please ensure your implementation does not depend on calls to getPointerElementType() -- you can determine the type based on load/store instructions.

@nikic  Thanks, why is this better? (half done BTW)



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp:746
+        // Add pass to promote arguments passed by reference
+        if (Level.getSpeedupLevel() >= OptimizationLevel::O3.getSpeedupLevel())
+          PM.addPass(MSSAArgPromotionPass());
----------------
arsenm wrote:
> Why require all the way to -O3? -O2?
I thought its quite "agressive" kind of opt, may be 02 should suite better.


================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:324-326
+             if (HasLoadOnEveryPath) dbgs() << "has a load on every path,";
+             else dbgs() << (Candidate.Preload ? "" : "not")
+                         << " all callers pass a valid dereferenceable ptr,");
----------------
arsenm wrote:
> Weird formatting
lint however tries to convince me the previous formatting was better.


================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:824
+#ifndef NDEBUG
+  MSSA.verifyMemorySSA();
+#endif
----------------
arsenm wrote:
> I would assume this happens in the regular verifier with expensive checks?
Those checks aren't enough here because I modify MSSA and then reuse it for the next promoted argument.


================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:1344
+                      false)
+INITIALIZE_PASS_DEPENDENCY(CallGraphWrapperPass)
+INITIALIZE_PASS_END(MSSAArgPromotion, "mssaargpromotion",
----------------
rampitec wrote:
> Don't you need to add at least MemorySSAWrapperPass and AAResultsWrapperPass?
Well I cannot use it because this is a CallGraph pass and those analyses are available per function. Instead I use FunctionAnalysisManager to get per function results, the drawback is that I have to invalidate it. However this is only for the legacy pass manager.


================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:1385
+        assert(OldNode->getNumReferences() == 0);
+        delete CG.removeFunctionFromModule(OldNode);
+        SCC.ReplaceNode(OldNode, NewNode);
----------------
arsenm wrote:
> It's weird to have raw deletes in llvm code, why do you need this here?
It was taken from ArgumentPromotion code, will take a look if its really needed.


================
Comment at: llvm/test/Transforms/ArgumentPromotion/inoutargs.ll:1103
+  ret i32 %Sum
+}
----------------
arsenm wrote:
> I'd like to see some tests with more exotic memory operations (atomics, memcpy/memset, target memory intrinsics etc.).
> 
> I also don't see negative tests for some of the skipped conditions (e.g. musttail). Can you also add a test for invoke, and captured function address
Right, will add more tests. However current implementation is quite conservative as relies on MSSA to find clobbers, it requires additional false-clobber checks as was done by Stas in https://reviews.llvm.org/D118419.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119013