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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D54498#1339322 <https://reviews.llvm.org/D54498#1339322>, @hfinkel wrote:

> > 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.
>
> Splitting the patch and providing multiple use cases does not necessarily require rewriting the IPConstProp without the abstraction. This can be split into multiple (dependent) patches and other dependent patches can also be posted showing other use cases.


I can certainly split this into more patches if that is preferred. I'm unsure though how to test the metadata and abstract call sites without a user.



================
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
----------------
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.


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

I don't think we want to connect general broker function side effects to callbacks in any way. The broker function side effects shall be arbitrary, if not otherwise specified by attributes. **HOWEVER**, the "passthrough" callback arguments are not to be inspected or modified, as stated in the documentation. This allows us to treat the callback as a "regular call" for the purpose of information transfer. Though, we never lose the actual broker function call and therefore will always be required to consider its side effects.

I can add a phrase stating this more clearly.


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

It is required for the OpenMP use case and I'm unsure why it complicates much. We do not really optimize based on what was executed "for sure", and even if we did, abstract call sites handling would need to be added explicitly.


================
Comment at: docs/LangRef.rst:5113
+
+
 '``unpredictable``' Metadata
----------------
reames wrote:
> 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.)
> 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 agree, but I'm not too familiar with them right now so I have to come back to you on this point.


> 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.)

The argument encoding has to be somewhere and I'm not sure why it shouldn't be at the broker functions. If we have it on the call sites exclusively, we end up duplicating it a lot and I'd like to hear why that might be worth it.


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


================
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) {
 
----------------
reames wrote:
> While correct, this would be a lot easier to read if you used F.arg_size() as the end condition.
I did not "write" this code, just kept it. I can use F.arg_size() if ppl prefer that.


================
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);
----------------
reames wrote:
> 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.)
Lets start here:

> This relates to the isCallee check removal.

If an abstract call site ACS for a use U is built, thus it evaluates to a boolean true (line 67), we know that:


  - The callee for the ACS is represented by U
  - The ACS will behave like a classical call site iff `CallSite::isCallee(U)` would return true.
  - The ACS is considered a callback callee with all the consequences iff `CallSite::isCallee(U)` would return false.


> why is it correct to adjust the operand numbers using the metadata provided offset?

We only adjust argument indices, at least we only should, if the abstract call site represents a callback call. This is the case only if the use that created it is not the `CallInst::isCallee()` use. So when "the broker function use" of the `CallInst` is processed, the ACS is not a callback call site and `AbstractCallSite::getCallArgOperand(int)` is the identify function. Does that make sense/help?




================
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,
----------------
reames wrote:
> 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.
I was reminded in a review that I might need it. And I think I do. It should only return true for globals that are thread local and these we should not propagate through "arbitrary callback call sites".


================
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.
----------------
reames wrote:
> 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?
I can get rid of the new line and the single space difference, both not intentional but probably caused by some subconscious code cleanup. There are no NFC indexing changes, I had to change the way we iterate to allow for the remapping of indices, that part has to stay.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54498