[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:2281
+    if (!CB)
+      continue;
+
----------------
Maybe assert any non-callbase use is a BlockAddress


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2288
+    for (auto *It = OpBundles.begin(); It != OpBundles.end(); ++It) {
+      if (It->getTag() == "preallocated") {
+        PreallocatedToken = *It->input_begin();
----------------
aeubanks wrote:
> I couldn't figure out how to remove the preallocated operand bundle any better than this, suggestions greatly appreciated
Looked briefly; I don't think there's any other way.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2304
+    if (CB->extractProfTotalWeight(W))
+      NewCB->setProfWeight(W);
+    CB->replaceAllUsesWith(NewCB);
----------------
Is the extractProfTotalWeight really necessary?  I would have thought copying a call using CallInst::Create() would copy that as well.


================
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) {
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80951