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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/IR/CallSite.h:697
+/// called function (=broker) may invoke a third one (=callback callee).
+/// In this case, the abstract call side hides the middle man, hence the
+/// broker function. The result is a representation of the callback call,
----------------
side -> site


================
Comment at: include/llvm/IR/CallSite.h:699
+/// broker function. The result is a representation of the callback call,
+/// inside the broker, but in the context of the original instruction.
+///
----------------
I think it's clearer if you say:

instruction -> call to the broker


================
Comment at: include/llvm/IR/CallSite.h:703
+/// sites. The caller (1), which invokes the broker function. The broker
+/// function (2), that may or may not invoke the callee. And finally the callee
+/// (3), which is the target of the callback call.
----------------
will invoke the callee zero or more times

(to cover the fact that it might also be invoked more than once)


================
Comment at: include/llvm/IR/CallSite.h:763
+  /// Return true if this ACS represents a callback call.
+  bool isCallbackCall() const {
+    // For a callback call site the callee is ALWAYS stored first in the
----------------
FWIW, I like 'Callback' because it's pretty self-documenting (most everyone knows what a callback is). Given your terminology above, it might also make sense to use BrokeredCall. Thus, if we need a backup name, I suggest that one.


================
Comment at: lib/IR/AbstractCallSite.cpp:82
+    KBF_UNKNOWN,
+    KBF_PTHREAD_CREATE, /// < pthread_create
+    KBF_KMPC_FORK_CALL, /// < __kmpc_fork_call
----------------
What about pthread_once?

As a higher-level comment, I'd rather not have a list here. It would be better to take this from metadata on the call and have Clang add the metadata for functions that it recognizes (and for the OpenMP runtime calls that it inserts).



Repository:
  rL LLVM

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

https://reviews.llvm.org/D54498