[PATCH] D18257: Add "first class" lowering for deopt operand bundles

[Philip Reames via llvm-commits]

reames added a comment.

Mostly minor comments.  This looks close to ready to go in.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2134
@@ +2133,3 @@
+            I.getOperandBundleAt(i).isFuncletOperandBundle()) &&
+           "Cannot lower invokes with arbitrary operand bundles!");
+#endif
----------------
add "yet" to comment?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2156
@@ -2148,2 +2155,3 @@
     }
-  } else
+  } else if (I.countOperandBundlesOfType(LLVMContext::OB_deopt)) {
+    LowerCallSiteWithDeoptBundle(&I, getValue(Callee), EHPadBB);
----------------
We talked about this offline, but just for the record...

This code is intentionally not handling deopt operands attached to any intrinsic call.  Support for @llvm.experimental.deoptimize will follow in another patch and we actively don't want to support deopt on other intrinsics at this time.

I'd add a comment to make that explicit.  

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6114
@@ +6113,3 @@
+#ifndef NDEBUG
+  for (unsigned i = 0, e = I.getNumOperandBundles(); i != e; ++i)
+    assert((I.getOperandBundleAt(i).isDeoptOperandBundle() ||
----------------
It currently looks like we're actually supporting *either* deopt operands XOR funclet operands.  I might remove the loop and just assert that nothing other than deopt operands are available on the deopt path.  

Same in the other place this loop exist.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:844
@@ +843,3 @@
+  StatepointLoweringInfo SI(DAG);
+  populateCallLoweringInfo(SI.CLI, CS, 0, CS.getNumArgOperands(), Callee,
+                           CS.getType(), false);
----------------
0 as a magic value is unclear.  At least pull out a named constant CallSiteArgOperandBegin.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:857
@@ +856,3 @@
+
+  StatepointDirectives SD =
+      parseStatepointDirectivesFromAttrs(CS.getAttributes());
----------------
Minor code structure: I'd move the default init immediately before each conditional init to make the value flow obvious.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:864
@@ +863,3 @@
+
+  SDValue ReturnVal = LowerAsStatepoint(SI);
+  if (ReturnVal.getNode()) {
----------------
Could this be written as:
if (SDValue ReturnVal = LowerAsStatepoint(SI))

Also, minor, I think it would be good to rename LowerAsStatepoint to either LowerAsSTATEPOINT or LowerAsStatepointPseudo.  I keep getting the IR and MI uses of the name when reading the new code structure.

================
Comment at: test/CodeGen/X86/deopt-bundles.ll:98
@@ +97,3 @@
+; CHECK:	movl	$45, %edi
+; CHECK:	nop
+; CHECK:	popq	%rcx
----------------
Er, this doesn't look like 9 patchable bytes?  Am I missing something?


http://reviews.llvm.org/D18257