[PATCH] D15340: Separate CSRs into a subset handled via explicit copies and a subset handled by prologue/epilogue

[hfinkel@anl.gov via llvm-commits]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

I'd like to make sure that I understand the situation:

You have functions that you'd like to shrink wrap, but you can't because you have calls to tlv_get_addr that post-dominate the entry, and tlv_get_addr clobbers the LR and FP registers. Normally, this requires LR and FP to be saved to the stack in the prologue, and here, you still do. And you still can't shrink wrap. But, instead of saving the other CSRs to the stack, you copy them to other vregs, the thought being that you'll spill them as needed anyway, and you might as well let the register allocator make that decision than blindly spilling them all up front.

Saving CSRs by copying normally does not make sense if you'll immediately spill them, but does make sense if you have high-register pressure only in some cold path. I don't understand what this has to do with special calling conventions for TLS, but it seems like a generally-good thing to do any time you have high register pressure in some (cold) part(s) of a function and low register pressure elsewhere. Do functions with this CXX_FAST_TLS calling convention always exhibit that pattern? Should we endeavor to detect it more generally?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:472
@@ +471,3 @@
+  // and the function has a single exit.
+  FuncInfo->SplitCSR = Fn.getCallingConv() == CallingConv::CXX_FAST_TLS &&
+                       TLI->supportSplitCSR();
----------------
Restricting this to CallingConv::CXX_FAST_TLS does not belong in target-independent code. Please move this into the TLI->supportSplitCSR() implementation.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:480
@@ +479,3 @@
+      bool IsReturn = !BB.empty() && isa<ReturnInst>(&BB.back());
+      if (succ_empty(&BB) || IsReturn) {
+        // If we already saw an exit, bail out. If this is an exit, but
----------------
Am I correct in assuming that the `succ_empty(&BB)` check is for blocks that end in, at the IR level, an unreachable? If so, I'm not sure this logic is right, because we should be able to have as many unreachables as we'd like, so long as we have only one actual return.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:493
@@ +492,3 @@
+
+    if (!SingleReturn)
+      FuncInfo->SplitCSR = false;
----------------
Fundamentally, why are we checking for a single return? Wouldn't you just insert the vreg->csr copies into each return block independently should there by more than one?


http://reviews.llvm.org/D15340