[PATCH] D80951: [GlobalOpt] Remove preallocated calls when possible

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks marked 4 inline comments as done.
aeubanks added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2281
+    if (!CB)
+      continue;
+
----------------
efriedma wrote:
> Maybe assert any non-callbase use is a BlockAddress
Done in newly added `hasMustTailCallers()`.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2304
+    if (CB->extractProfTotalWeight(W))
+      NewCB->setProfWeight(W);
+    CB->replaceAllUsesWith(NewCB);
----------------
efriedma wrote:
> Is the extractProfTotalWeight really necessary?  I would have thought copying a call using CallInst::Create() would copy that as well.
Did a little test, yes it's required. Do you think it makes sense to fix this globally?


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2311
+    // Cannot modify users() while iterating over it, so make a copy.
+    SmallVector<User *, 4> PreallocatedArgs(PreallocatedToken->users());
+    for (auto *User : PreallocatedArgs) {
----------------
efriedma wrote:
> I think it's possible to have multiple llvm.call.preallocated.arg() calls with the same index?
> 
> If you're creating dynamic allocas, you probably want to llvm.stacksave/stackrestore in appropriate places.
> I think it's possible to have multiple llvm.call.preallocated.arg() calls with the same index?
Hmm, that's true. I guess for each index that is used, create an alloca right after the llvm.call.preallocated.setup() and replace the corresponding llvm.call.preallocated.arg with the result of that. That way the alloca will dominate all its uses.
> 
> If you're creating dynamic allocas, you probably want to llvm.stacksave/stackrestore in appropriate places.
> 
Oh I though llvm automatically handled dynamic allocas, I'll look more into this.



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2405
+    if (F->getAttributes().hasAttrSomewhere(Attribute::Preallocated) &&
+        !F->hasAddressTaken()) {
+      RemovePreallocated(F);
----------------
efriedma wrote:
> Do you also need to check for musttail calls?
Good point, done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80951