[PATCH] D119013: [ArgPromotion][AMDGPU] New MSSA-based function argument promotion pass with input/output argument support
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Feb 4 11:06:24 PST 2022
arsenm added a comment.
This looks like a better version of AMDGPURewriteOutArguments which was never enabled. I wanted to replace that to use MSSA
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?
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:175
+ bool isMyLoad(Value *V) const {
+ return isa<LoadInst>(V) && isMyPtr(cast<LoadInst>(V)->getPointerOperand());
+ }
----------------
dyn_cast instead of isa and cast
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:178
+ bool isMyStore(Value *V) const {
+ return isa<StoreInst>(V) &&
+ isMyPtr(cast<StoreInst>(V)->getPointerOperand());
----------------
dyn_cast instead of isa and cast
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:184
+ return isMyPtr(LI->getPointerOperand());
+ else if (StoreInst *SI = dyn_cast<StoreInst>(V))
+ return isMyPtr(SI->getPointerOperand());
----------------
No else after return
================
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,");
----------------
Weird formatting
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:813
+ for (BasicBlock &BB : *F)
+ if (auto *RetInst = dyn_cast_or_null<ReturnInst>(BB.getTerminator()))
+ RetValues[RetInst].push_back(getBBExitValue(&BB));
----------------
The terminator can't be null
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:824
+#ifndef NDEBUG
+ MSSA.verifyMemorySSA();
+#endif
----------------
I would assume this happens in the regular verifier with expensive checks?
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:856
+ const ArgPromotionInfo *A2) const {
+ assert(!(A1->isClobberedBy(*A2) && A2->isClobberedBy(*A1)));
+ return A1->isClobberedBy(*A2);
----------------
Demorgan this
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:912
+ Queue.push(&C);
+ // otherwise this is a circular dependency, other candidates will be
+ // removed by the condition [1]
----------------
Capitalize
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:928
+ ReturnArgs.reserve(NumPromoted);
+ for (ArgPromotionInfo &C : Candidates)
+ if (C.isPromoted()) {
----------------
Braces
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:954
+ unsigned I = 0;
+ if (!OldRetTy->isVoidTy())
+ RetValue = InsertValueInst::Create(
----------------
Braces
================
Comment at: llvm/lib/Transforms/IPO/MSSAArgPromotion.cpp:1385
+ assert(OldNode->getNumReferences() == 0);
+ delete CG.removeFunctionFromModule(OldNode);
+ SCC.ReplaceNode(OldNode, NewNode);
----------------
It's weird to have raw deletes in llvm code, why do you need this here?
================
Comment at: llvm/test/Transforms/ArgumentPromotion/inoutargs.ll:1103
+ ret i32 %Sum
+}
----------------
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
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119013/new/
https://reviews.llvm.org/D119013
More information about the llvm-commits
mailing list