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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
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
----------------
jdoerfert wrote:
> reames wrote:
> > 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.  
> > 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 thought I did that in the commit message and in the documentation/examples below.
> 
> 
> > I think you'd do well to introduce the concepts separately from the semantic description.
> 
> I'm unsure about this but if people think we need an explicit documentation point on these I can do it. (So far,) They only exist in the context of callbacks, so I described them in that context only. 
> 
> 
> > I don't see any reason to exclude metadata on call sites.
> 
> Me neither. Though, for now, it seems sufficient to put them on the brokers and, as I mention below again, I only see upsides in doing so. Especially we only require a single argument encoding. Once we have use cases that require different argument encodings, or we prefer call sites for some other reason, we can simply lift this restriction.
Johannes and I talked offline.  Here's the suggested redrafting I promised:

``callback`` metadata may be attached to a function declaration, or definition.  For ease of exposition, we'll refer to the function annotated w/metadata as a broker.  The metadata describes how the arguments of a call to the broker function are in turn passed to the specified callback function when invoked by. the broker.  The only semantic restriction on the broker function itself is that it is assumed not to inspect or modify arguments passed to the callback function. 

(later in detailed semantics)

The broker is not required to actually invoke the callback function dynamically.  However, the assumptions about not inspecting or modifying arguments that would be passed to the specified callback function still hold, even if the callback function is not dynamically invoked.  The broker is allowed to invoke the callback function more than once per invocation of the broker.  The broker is also allowed to invoke (directly or indirectly) the function passed as a callback through another use.  However, the pass through argument semantics are assumed to apply to all invocations of the callback function by the broker.  (Mostly because we have no way to distinguish said calls after potential inter-procedural constant folding of the function pointer itself.)

(end proposed drafting)

General comment: I'd pull the definition of the metadata encoding out before introducing the example.  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54498