[PATCH] D14552: Teach the inliner to track deoptimization state

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Replies inline.

Summarizing my plan to address the more complex review commments, I've proposed three post-commit changes inline:

- Change `OperandBundleDef` such that it can (alternative to containing an `std::string` tag name) contain an integral tag ID
- Change `CALLSITE_DELEGATE_SETTER` and `CALLSITE_DELEGATE_GETTER` to use the safer `do { ... } while (false)` idiom
- Introduce an iterator for `getOperandBundleAt(I)` and use that as much as possible


================
Comment at: include/llvm/IR/CallSite.h:215
@@ -214,3 @@
-  InstrTy *II = getInstruction();    \
-  return isCall()                        \
-    ? cast<CallInst>(II)->METHOD         \
----------------
reames wrote:
> This really looks like a white space change?  
> 
> If you're going to introduce control flow, please use the "do { X } while(false)" idiom to make it less prone to miscompies.  
I'm just copying the pattern from `CALLSITE_DELEGATE_SETTER`.  I agree that the `do { ... } while (false)` form is better -- is it okay if I fix `CALLSITE_DELEGATE_SETTER` and `CALLSITE_DELEGATE_GETTER` together in a separate checkin?

================
Comment at: include/llvm/IR/Instructions.h:1463
@@ +1462,3 @@
+
+  /// \brief Clone this call instruction with a different set of operand
+  /// bundles.
----------------
reames wrote:
> This comment is unclear.  Is the argument list a white list?  Is it an entirely new list of operand bundles?  If so, where'd it come from?
It is an entirely new list of operand bundles.  Will update the doc.

================
Comment at: lib/IR/Instructions.cpp:301
@@ +300,3 @@
+CallInst *CallInst::cloneWithOperandBundles(ArrayRef<OperandBundleDef> OpB) {
+  std::vector<Value *> Args(op_begin(), op_begin() + getNumArgOperands());
+  auto *CI = CallInst::Create(getCalledValue(), Args, OpB, getName());
----------------
reames wrote:
> Don't we have an args_begin(), args_end?
> 
> Or better, an args() which returns an ArrayRef?
They're there on `CallSite`, not on `CallInst`.  Does changing this to

```
std::vector<...> Args = CallSite(this).args()
```

sound reasonable?

================
Comment at: lib/IR/Instructions.cpp:585
@@ +584,3 @@
+InvokeInst::cloneWithOperandBundles(ArrayRef<OperandBundleDef> OpB) {
+  std::vector<Value *> Args(op_begin(), op_begin() + getNumArgOperands());
+  auto *II = InvokeInst::Create(getCalledValue(), getNormalDest(),
----------------
reames wrote:
> Same as above?
Same reply as above.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:211
@@ -210,3 +210,3 @@
     SmallVector<Value*, 8> InvokeArgs(CS.arg_begin(), CS.arg_end());
-    InvokeInst *II = InvokeInst::Create(CI->getCalledValue(), Split, UnwindEdge,
-                                        InvokeArgs, CI->getName(), BB);
+    SmallVector<OperandBundleDef, 1> OpBundles;
+    for (unsigned i = 0, e = CS.getNumOperandBundles(); i != e; ++i)
----------------
reames wrote:
> This really seems like we need a getOperandBundles that returns an ArrayRef?
That's difficult -- the object returned by `getOperandBundleAt` is a flyweight object (like `StringRef` etc.) that does not have a direct backing store (i.e. there is no internal array of `OperandBundleUse` objects) that I can create an `ArrayRef<OperandBundleUse>` with.  I can definitely write a stateful iterator that iterates through all the operand bundles for a CallSite, but I've been waiting to do that sort of thing after bring-up.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1039
@@ +1038,3 @@
+  if (CS.hasOperandBundles()) {
+    bool CanInline =
+        CS.getNumOperandBundles() == 1 &&
----------------
reames wrote:
> I'd just inline this into the if and swap the comparison directions.
> 
> p.s. Don't you need to worry about a bundle type in the callee which is not deopt?
> I'd just inline this into the if and swap the comparison directions.

SGTM.

> p.s. Don't you need to worry about a bundle type in the callee which is not deopt?

The current semantics is that the caller's deopt state gets prepended to all of the `"deopt"` operand bundles in the callee.  Call sites in the callee that have arbitrary deopt operand bundles are left intact.  The test cases demonstrate this.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1151
@@ -1140,1 +1150,3 @@
 
+    if (CS.hasOperandBundles()) {
+      auto ParentDeopt = CS.getOperandBundleAt(0);
----------------
reames wrote:
> I'm not sure about the order of the code here, but the fact we're replacing the same instruction twice (once for call to invoke, once for deopt bundles) seems likely to be error prone, particularly with ValueHandles being involved.  Is there a way to avoid the complexity here?  Don't have an obvious one myself, just asking.  
I don't think there should be problems with `ValueHandle` s, but I do think this is somewhat inefficient.  However, I don't think how I can fix that without changing `CloneFunction` to be aware of whether it is inlining into a call site that's a `call` instruction, or an `invoke`; and changing that is outside the scope of this patch.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1185
@@ +1184,3 @@
+
+          OpDefs.emplace_back(ChildOB.getTagName(), std::move(MergedDeoptArgs));
+        }
----------------
reames wrote:
> TagName?  Why not tagID?  And why not hard code OB_deopt here since it's implied by the surrounding code?
Right now the `OperandBundleDef` structure only carries an `std::string` that gets re-interned into `LLVMContextImpl` when the host instruction is constructed -- it has no  way of holding a direct integer ID.  This needs to be fixed (for performance, if nothing else), but doing that in this change will make it more complex.  How about I change `ChildOB.getTagName()` to `"deopt"` for now, and in a later change add the functionality to create an `OperandBundleDef` holding a direct integral ID?


http://reviews.llvm.org/D14552