[PATCH] D54498: AbstractCallSite -- A unified interface for (in)direct and callback calls

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

A high level strategy comment.  I think you're trying to go a bit too far in one patch.  I'd advise first writing a patch which does nothing but implement the IPConstProp adjustments "the hard way" (i.e. without the AbstractCallSite) and focus on defining the metadata with the semantics you once.  To really justify the complexity of the abstraction, you need more than one user.  You could add another user to this patch, but doing so will just slow down the review.  Separating this into a series of patches (IPConstProp w/metadata def, second user, then introduce abstraction) will make it much easier to see the necessity and make each piece more easily reviewable.



================
Comment at: include/llvm/IR/CallSite.h:843
+    Value *V = getCalledValue();
+    return V ? dyn_cast<Function>(V->stripPointerCasts()) : nullptr;
+  }
----------------
reames wrote:
> dyn_cast_or_null
p.s. Ignore this suggestion.  It's buggy due to the need for stripPointerCasts


================
Comment at: lib/Transforms/IPO/IPConstantPropagation.cpp:73
     Function::arg_iterator Arg = F.arg_begin();
-    for (unsigned i = 0, e = ArgumentConstants.size(); i != e;
-         ++i, ++AI, ++Arg) {
+    for (unsigned i = 0, e = ArgumentConstants.size(); i != e; ++i, ++Arg) {
 
----------------
While correct, this would be a lot easier to read if you used F.arg_size() as the end condition.


================
Comment at: lib/Transforms/IPO/IPConstantPropagation.cpp:79
 
-      Constant *C = dyn_cast<Constant>(*AI);
+      Value *V = ACS.getCallArgOperand(i);
+      Constant *C = dyn_cast_or_null<Constant>(V);
----------------
Ok, I'm not following a key detail here.  If we're processing a call to the broker function with a 5 element argument list (let's say), why is it correct to adjust the operand numbers using the metadata provided offset?  I could see how that would be correct when visiting users of the *callback* function, but don't we end up invoking this function for the *broker* function as well?  (This relates to the isCallee check removal.)


================
Comment at: lib/Transforms/IPO/IPConstantPropagation.cpp:83
+      // We can only propagate thread independent values through callbacks.
+      if (C && ACS.isCallbackCall() && C->isThreadDependent()) {
+        // Argument became non-constant. If all arguments are non-constant now,
----------------
isThreadDependent on a constant is a new one to me.  Given I see a total of three uses in all of LLVM, I suspect this hasn't been fully plumbed through.


================
Comment at: lib/Transforms/IPO/IPConstantPropagation.cpp:100
       } else {
-        // Argument became non-constant.  If all arguments are non-constant now,
+        // Argument became non-constant. If all arguments are non-constant now,
         // give up on this function.
----------------
You have some assorted white space and NFC indexing changes here.  Would you mind separating those and landing them so they don't clutter the review?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54498