[PATCH] Remap frame variables used in outlined C++ exception handlers

[Andy Kaylor]

Thanks for the quick review.  I'll clean this up and submit an update tomorrow.


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:43
@@ +42,3 @@
+// frame allocation structure.
+typedef MapVector<AllocaInst *, std::pair<SmallVector<AllocaInst *, 1>, int>>
+    FrameVarInfoMap;
----------------
rnk wrote:
> SmallVectors don't nest in other containers very efficiently. You can use a TinyPtrVector instead, which just one pointer, and can be moved very cheaply during a container reallocation.
> 
> I'm also partial to just making a struct for the map values instead of using std::pair, along the lines of:
>   struct HandlerAllocas {
>     TinyPtrVector<AllocaInst> Allocas;
>     int ParentFrameAllocationIndex;
>   };
Sounds good.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:245
@@ +244,3 @@
+  int Idx = 2;
+
+  // Map the alloca instructions to the corresponding index in the
----------------
rnk wrote:
> Can we sort FrameVarInfo by the alignment and size of ParentAlloca first? It doesn't have to be this patch, but a FIXME here would be good. Since we're freezing the stack layout here during preparation, we can't rely on downstream stack layout passes doing it for us. =/
Yeah, that definitely needs to happen.  I think we'll also need to insert some padding to maintain alignment.  I'll add a FIXME.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:256-258
@@ +255,5 @@
+  // problem.
+  for (FrameVarInfoMap::iterator I = FrameVarInfo.begin(),
+                                 E = FrameVarInfo.end();
+       I != E; I++) {
+    AllocaInst *ParentAlloca = I->first;
----------------
rnk wrote:
> Range-based for? I think `for (const auto &KeyValue : FrameVarInfo)` will do the trick.
I was avoiding that because the type declaration was so ridiculous.  I didn't think about auto.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:328-330
@@ +327,5 @@
+  // that get the equivalent pointer from the frame allocation struct.
+  for (FrameVarInfoMap::iterator I = FrameVarInfo.begin(),
+                                 E = FrameVarInfo.end();
+       I != E; I++) {
+    AllocaInst *ParentAlloca = I->first;
----------------
rnk wrote:
> Range for?
Sure.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:347-348
@@ +346,4 @@
+      Builder.SetCurrentDebugLocation(ParentAlloca->getDebugLoc());
+      Value *ElementPtr =
+          Builder.CreateConstInBoundsGEP2_32(FrameEHData, 0, Idx);
+      ParentAlloca->replaceAllUsesWith(ElementPtr);
----------------
rnk wrote:
> Ultimately, to reduce register pressure and improve address mode matching, we'll want to sink these GEPs into the basic blocks of their uses. Normally, we could rely on CodeGenPrepare to do that for us, but it runs before EH preparation, so we can't.
> 
> We don't have to do that now, but a FIXME would be good. We should be able to write a helper function that iterates the use-list and inserts a GEP prior to each instruction.
OK.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:605-606
@@ +604,4 @@
+
+// FIXME: This doesn't work during cloning because it finds an instruction
+//        in the use list that isn't yet part of a basic block.
+#if 0
----------------
rnk wrote:
> This is pushing me in the direction of trying to do an upfront analysis that identifies blocks in need of outlining instead of discovering blocks as we go.
I have code in my work-in-progress sandbox that does the upfront analysis.  I don't believe that handles this case though.  I think the problem here is that we've cloned an instruction that uses this variable, but the cloned instruction won't be placed in a BasicBlock until this materialization is done.  That shouldn't be difficult to handle, but I didn't want to hold up this review waiting for it.

http://reviews.llvm.org/D7770

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/