[PATCH] D18429: Add lowering support for llvm.experimental.deoptimize

[Philip Reames via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/comments addressed.


================
Comment at: docs/LangRef.rst:12174
@@ +12173,3 @@
+symbol ``__llvm_deoptimize`` (it is the frontend's responsibility to
+ensure that this symbol is available).  The call arguments to the
+``@llvm.experimental.deoptimize`` call are passed via a non-varargs
----------------
"is available" to "is defined."

================
Comment at: docs/LangRef.rst:12175
@@ +12174,3 @@
+ensure that this symbol is available).  The call arguments to the
+``@llvm.experimental.deoptimize`` call are passed via a non-varargs
+calling convention to ``__llvm_deoptimize``.
----------------
non-varargs is a bit vague here.  "Lowered according to the calling convention specified for the deoptimize call as if they were formal arguments of the specified types not vararg."  Wording could use improved, but that gets the spirit.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:925
@@ +924,3 @@
+void SelectionDAGBuilder::LowerDeoptimizeCall(ImmutableCallSite CS) {
+  const auto &TLI = DAG.getTargetLoweringInfo();
+
----------------
It looks like you're only handling calls here.  Using CallSite is fine, but add an assertion.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:940
@@ +939,3 @@
+
+  unsigned DefaultID = StatepointDirectives::DeoptBundleStatepointID;
+
----------------
Not sure the exact variable adds anything here.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:949
@@ +948,3 @@
+  SI.StatepointFlags = static_cast<uint64_t>(StatepointFlags::None);
+
+  if (SDValue ReturnVal = LowerAsSTATEPOINT(SI)) {
----------------
You might add a comment to make it clear GC args intentionally remain empty.

================
Comment at: test/CodeGen/X86/deopt-intrinsic.ll:2
@@ +1,3 @@
+; RUN: llc < %s | FileCheck %s
+; RUN: llc -debug-only=stackmaps < %s 2>&1 | FileCheck --check-prefix=STACKMAPS %s
+
----------------
Doesn't -debug-only require an asserts build?

================
Comment at: test/CodeGen/X86/deopt-intrinsic.ll:12
@@ +11,3 @@
+; CHECK: _caller_0:
+; CHECK-NOT: mov
+; CHECK: callq	___llvm_deoptimize
----------------
Might be better to just check for each expected instruction.

================
Comment at: test/CodeGen/X86/deopt-intrinsic.ll:13
@@ +12,3 @@
+; CHECK-NOT: mov
+; CHECK: callq	___llvm_deoptimize
+entry:
----------------
Add a check-next for the return.  The fact there's nothing in between is worth checking.

================
Comment at: test/CodeGen/X86/deopt-intrinsic.ll:21
@@ +20,3 @@
+; CHECK: _caller_1
+; CHECK: movss	{{[a-zA-Z0-9_]+}}(%rip), %xmm0    ## xmm0 = mem[0],zero,zero,zero
+; CHECK-NEXT: movl	$42, %edi
----------------
Is this how we materialize a floating point constant?  That seems like it could be improved in general.

================
Comment at: test/CodeGen/X86/deopt-intrinsic.ll:26
@@ +25,3 @@
+entry:
+  %v = call i8(...) @llvm.experimental.deoptimize.i8(i32 42, float 500.0) [ "deopt"(i32 1) ]
+  ret i8 %v
----------------
Can you add a test with a different calling convention?  There's nothing here that ensures we're using the right one for the lowering as opposed to merely the default.


http://reviews.llvm.org/D18429