[PATCH] D125485: [ArgPromotion] Unify byval promotion with non-byval

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Sorry for the long delay here. This looks pretty nice!



================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:322
+  // after the following loop
+  std::vector<AllocaInst *> Allocas;
+  Allocas.reserve(std::accumulate(ArgsToPromote.begin(), ArgsToPromote.end(),
----------------
`SmallVector<AllocaInst *>`


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:326
+                                    return Res + ArgParts.second.size();
+                                  }));
+
----------------
I doubt this code is performance-critical enough to need this accumulate + reserve :)


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:370
+          Part.Ty, nullptr, Arg.getName() + "." + Twine(Offset) + ".allc");
+      IRB.CreateAlignedStore(NewArg, NewAlloca, Pair.second.Alignment, false);
+
----------------
Can drop the `false` argument, non-volatile store is the default.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:376
 
-    // Otherwise, if we promoted this argument, then all users are load
-    // instructions (with possible casts and GEPs in between).
+    // Cleanup the code from the dead instructions: GEPs and BitCasts in between
+    // the original argument and its users: loads and stores. Retarget every
----------------
This comment looks a bit out of place, as this closure doesn't remove any dead instructions itself.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:384
+      assert(Ptr == &Arg && "Not constant offset from arg?");
+      return OffsetToAlloca[Offset.getSExtValue()];
+    };
----------------
Mild preference for `OffsetToAlloca.lookup()` here, which makes it clear that we don't intend to modify the map (which `operator[]` can do).


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:401
+        Value *TheAlloca = GetAlloca(Ptr);
+        IRBuilder<NoFolder> LIRB(LI);
+        Instruction *NewLI = LIRB.CreateAlignedLoad(LI->getType(), TheAlloca,
----------------
It's possible to reuse one IRBuilder instance using `IRB.SetInsertPoint()`.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:403
+        Instruction *NewLI = LIRB.CreateAlignedLoad(LI->getType(), TheAlloca,
+                                                    LI->getAlign(), "");
+        NewLI->takeName(LI);
----------------
Can omit empty name, it's the default.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:415
+        Instruction *NewSI = SIRB.CreateAlignedStore(
+            SI->getValueOperand(), TheAlloca, SI->getAlign(), SI->isVolatile());
+        SI->replaceAllUsesWith(NewSI);
----------------
Volatile operations can't be promoted, can assert that `!SI->isVolatile()`.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:431
+    for (const auto &Pair : OffsetToAlloca) {
+      if (isAllocaPromotable(Pair.second))
+        Allocas.push_back(Pair.second);
----------------
Can we `assert(isAllocaPromotable())` here? I think that by design, we should only produce promotable allocas.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:498
 
-  // Returns None if this load is not based on the argument. Return true if
-  // we can promote the load, false otherwise.
-  auto HandleLoad = [&](LoadInst *LI,
-                        bool GuaranteedToExecute) -> Optional<bool> {
-    // Don't promote volatile or atomic loads.
-    if (!LI->isSimple())
+  // And if this is a byval argument we also allow to have the store
+  // instructions. Only handle in such way arguments with specified alignment;
----------------
The "the" can be dropped here.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:502
+  // target-specific.
+  bool IsStoresAllowed = Arg->getParamByValType() && Arg->getParamAlign();
+
----------------
IsStoresAllowed -> AreStoresAllowed


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:551
       LLVM_DEBUG(dbgs() << "ArgPromotion of " << *Arg << " failed: "
-                        << "loaded via both " << *Part.Ty << " and " << *Ty
+                        << (isa<LoadInst>(I) ? "loaded via" : "stored to")
+                        << " both " << *Part.Ty << " and " << *Ty
----------------
I would replaced this with "accessed as" to cover both. Otherwise it sounds like it's either both loads or both stores with conflicting types, but it can also be a mix.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:588
+      Res = HandleEndUser(LI, LI->getType(), /* GuaranteedToExecute */ true);
+    if (StoreInst *SI = dyn_cast<StoreInst>(&I))
+      Res = HandleEndUser(SI, SI->getValueOperand()->getType(),
----------------
else if


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:641
+      // Only stores TO the argument is allowed, all the other stores are
+      // unknown users
+    }
----------------
What happens if the we're storing the argument into itself? `store ptr %arg, ptr %arg` or similar. I don't think your code handles this correct right now?

It might be best to walk over `Use`s rather than `User`s, so we can check which operand the use is on.


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:671
   // a correctness perspective. Profitability is less obvious though.
+  // TODO: What about store promotion?
   int64_t Offset = ArgPartsVec[0].first;
----------------
I think it's safe to drop these TODOs -- this is not really compatible with the new promotion approach that uses separate allocas (we'd have to reconstruct the result from multiple allocas, which makes little sense).


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:635
+              HandleEndUser(SI, SI->getValueOperand()->getType(),
+                            /* GuaranteedToExecute */ true)) {
+        if (!*Res)
----------------
psamolysov wrote:
> I'm not aware about speculative stores. Should we handle the stores in the exactly same way as loads here, so should the parameter be set to `false`?
I think the parameter should be false here, because the store is not guaranteed to executed, and it would be confusing otherwise.

The relevant part here is that we're not going to speculate any stores, so we don't care about the alignment at all -- we can explicitly skip the alignment code in HandleEndUser for stores to make this clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125485