[PATCH] D112290: [Attributor] Look through indirect calls

[Shilei Tian via Phabricator via llvm-commits]

tianshilei1992 added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3303
   /// Return true if this AA requires a "callee" (or an associted function) for
   /// a call site positon. Default is optimistic to minimize AAs.
+  static bool requiresCalleeForCallBase() { return false; }
----------------
I thought the optimistic condition would be we don't have indirect calls such that the "callee" is required.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1015
+  for (Function *Callee : Callees)
+    if (!Pred(*Callee, Callees.size() == 1))
+      return false;
----------------
jdoerfert wrote:
> tianshilei1992 wrote:
> > jdoerfert wrote:
> > > arsenm wrote:
> > > > Why the compare to 1 for a bool?
> > > If we have more than 1 potential callee we don't know if it is a "must" callee anymore. I haven't thought enough about this to be sure we need or don't need it but there is the possibility we want to distinguish callees that may be called here from the one that certainly is.
> > Probably instead of using `NumCallees` (I'm not sure if the CB will need the value), can we use something like `bool IsUnique`?
> I figured it doesn't cost anything to provide more information. Once can use it for a heuristic, so decide not to spend time if the #callees is larget han some threshold.
I kinda think the interface of the CB just looks a little weird. Essentially the best way is something like `ArrayRef<Function *> Callees`, and then we can have both information. However, `checkForAll*` interfaces itself mean doing the check one by one instead of passing an array to the CB.
That being said, I don't have a strong objection to it, but just prefer a clean definition. If in the future we will need the number, we can revise it.


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

https://reviews.llvm.org/D112290