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

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: docs/LangRef.rst:1541
@@ -1510,1 +1540,3 @@
+    }
+
 .. _moduleasm:
----------------
I'd add something here about it being the frontend responsibility to ensure that the bundle format is sufficient so that individual abstract frames can be reconstructed from the concatenated form if required.  

================
Comment at: include/llvm/IR/CallSite.h:215
@@ -214,3 @@
-  InstrTy *II = getInstruction();    \
-  return isCall()                        \
-    ? cast<CallInst>(II)->METHOD         \
----------------
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.  

================
Comment at: include/llvm/IR/Instructions.h:1463
@@ +1462,3 @@
+
+  /// \brief Clone this call instruction with a different set of operand
+  /// bundles.
----------------
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?

================
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());
----------------
Don't we have an args_begin(), args_end?

Or better, an args() which returns an ArrayRef?

================
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(),
----------------
Same 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)
----------------
This really seems like we need a getOperandBundles that returns an ArrayRef?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1039
@@ +1038,3 @@
+  if (CS.hasOperandBundles()) {
+    bool CanInline =
+        CS.getNumOperandBundles() == 1 &&
----------------
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?

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1151
@@ -1140,1 +1150,3 @@
 
+    if (CS.hasOperandBundles()) {
+      auto ParentDeopt = CS.getOperandBundleAt(0);
----------------
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.  

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1175
@@ +1174,3 @@
+
+          assert(!Found && "Only one deopt operand bundle allowed!");
+          Found = true;
----------------
This is already enforced by the verifier, please remove.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1185
@@ +1184,3 @@
+
+          OpDefs.emplace_back(ChildOB.getTagName(), std::move(MergedDeoptArgs));
+        }
----------------
TagName?  Why not tagID?  And why not hard code OB_deopt here since it's implied by the surrounding code?


http://reviews.llvm.org/D14552