[PATCH] D17732: Introduce @llvm.experimental.deoptimize

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Re: doc comments -- I think did address all of them, can you point out the ones that you are not okay with in the current form?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5447
@@ -5446,1 +5446,3 @@
+  case Intrinsic::experimental_deoptimize:
+    return "__llvm_deoptimize";
   }
----------------
reames wrote:
> possibly we should have experimental in the name?  
> __llvm_experimental_deoptimize?
Good idea, will fix.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1814
@@ +1813,3 @@
+      auto NewEnd = remove_if(Returns, [](ReturnInst *RI) {
+        return RI->getParent()->getTerminatingDeoptimizeCall();
+      });
----------------
reames wrote:
> minor: the implicit bool conversion here is confusing.  nullptr != X would be clearer.  Alternatively, have the lambda explicitly return bool.
SGTM, will fix shortly.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1829
@@ +1828,3 @@
+
+        Value *CallArg = *DeoptCall->arg_begin();
+        assert((DeoptCall->arg_end() - DeoptCall->arg_begin()) == 1 && "Only one call argument allowed");
----------------
reames wrote:
> I'd intended in my previous comment to indicate that the intrinsic should take an unlimited number of untyped arguments.  Not one untyped argument.  
I thought about that -- having a varargs signature is not more general (since you can just create an aggregate type / alloca and pass that in), and did not feel like it was worth the complexity (e.g. we'd probably not want the call to be a varargs call at the ABI level).


http://reviews.llvm.org/D17732