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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Partial review, been on my queue too long, here's at least a few comments to help drive this forward.



================
Comment at: docs/LangRef.rst:5072
+
+``callback`` metadata may be attached to function definitions and declarations.
+The metadata specifies a callback callee and the callback arguments. If
----------------
A common theme running through this review is the use of the terms "callback" and "broker".  In the documentation, you need to find a convenient place to describe these two, given an example, and otherwise explain the concepts to the reader.  I struggled at first with even your patch description in being not quite sure what you meant by each.  You have some of this inlined into the semantic description, but I think you'd do well to introduce the concepts separately from the semantic description.  

I don't see any reason to exclude metadata on call sites. 

p.s. I think we'll likely end up changing these, but I'll treat the names as placeholders for the moment.  


================
Comment at: docs/LangRef.rst:5112
+    !0 = !{i1 true, i64 2, i64 -1, i64 -1}
+
+
----------------
One major piece missing from this is a discussion of what side effects (if any) the broker function is allowed to have.

Also, why the zero or more times thing?  Zero seems to complicate a lot of the reasoning.


================
Comment at: docs/LangRef.rst:5113
+
+
 '``unpredictable``' Metadata
----------------
We have the existing concepts of patchpoint, and statepoint which are both "brokers" in this terminology.  Would be good to think about how to represent that. 

I also see parallels to bundle operads on calls.  Maybe another approach to consider would be to represent the broker as a special bundle type on the call site to the callback?  (Not sure this works, but talking about why not might be interesting.)


================
Comment at: include/llvm/IR/CallSite.h:843
+    Value *V = getCalledValue();
+    return V ? dyn_cast<Function>(V->stripPointerCasts()) : nullptr;
+  }
----------------
dyn_cast_or_null


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54498