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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2304
+    if (CB->extractProfTotalWeight(W))
+      NewCB->setProfWeight(W);
+    CB->replaceAllUsesWith(NewCB);
----------------
aeubanks wrote:
> 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?
Maybe worth doing a brief survey of other callers.  I expect usually, you'd want to preserve almost all most metadata.


================
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) {
----------------
aeubanks wrote:
> 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.
> 
If you don't do anything, the "automatic" handling for dynamic allocas is that they remain live until the function returns, similar to alloca() in C.  That's probably not what you want here; you want the memory to be deallocated after the call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80951