[PATCH] D15139: [IR] Reformulate LLVM's EH funclet IR

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: lib/Analysis/CFG.cpp:241
@@ +240,3 @@
+
+DenseMap<BasicBlock *, ColorVector> llvm::colorEHFunclets(Function &F) {
+  SmallVector<std::pair<BasicBlock *, BasicBlock *>, 16> Worklist;
----------------
CFG.cpp is a bad place for this. We should probably create lib/Analysis/EHPersonalities.cpp or something. The personality classification that's currently in LibCallSemantics.cpp can live here too. We can do that move in-tree and then rebase this patch on that.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:321
@@ +320,3 @@
+
+static void calculateCXXStateNumbers(WinEHFuncInfo &FuncInfo,
+                                     const Instruction *FirstNonPHI,
----------------
Can you move this after getEHPadFromPredecessor so that the diff against the old code is smaller? I don't see an obvious reason why we have to move this down here.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:183-203
@@ -180,19 +182,23 @@
 
+static bool shouldCallConvertToInvoke(const CallInst *CI) {
+  // If this call cannot unwind, don't convert it to an invoke.
+  // Inline asm calls cannot throw.
+  return !CI->doesNotThrow() && !isa<InlineAsm>(CI->getCalledValue());
+}
+
 /// When we inline a basic block into an invoke,
 /// we have to turn all of the calls that can throw into invokes.
 /// This function analyze BB to see if there are any calls, and if so,
 /// it rewrites them to be invokes that jump to InvokeDest and fills in the PHI
 /// nodes in that block with the values specified in InvokeDestPHIValues.
 static BasicBlock *
 HandleCallsInBlockInlinedThroughInvoke(BasicBlock *BB, BasicBlock *UnwindEdge) {
   for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E; ) {
     Instruction *I = &*BBI++;
 
     // We only need to check for function calls: inlined invoke
     // instructions require no special handling.
     CallInst *CI = dyn_cast<CallInst>(I);
 
-    // If this call cannot unwind, don't convert it to an invoke.
-    // Inline asm calls cannot throw.
-    if (!CI || CI->doesNotThrow() || isa<InlineAsm>(CI->getCalledValue()))
+    if (!CI || !shouldCallConvertToInvoke(CI))
       continue;
----------------
This change seems superfluous? You don't use the helper elsewhere.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:261-264
@@ -254,6 +260,6 @@
   // Get all of the inlined landing pad instructions.
-  SmallPtrSet<LandingPadInst*, 16> InlinedLPads;
+  SmallPtrSet<LandingPadInst *, 16> InlinedLPads;
   for (Function::iterator I = FirstNewBlock->getIterator(), E = Caller->end();
        I != E; ++I)
-    if (InvokeInst *II = dyn_cast<InvokeInst>(I->getTerminator()))
+    if (auto *II = dyn_cast<InvokeInst>(I->getTerminator()))
       InlinedLPads.insert(II->getLandingPadInst());
----------------
We don't need these changes

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:338
@@ -331,3 +337,3 @@
   // Forward EH terminator instructions to the caller's invoke destination.
   // This is as simple as connect all the instructions which 'unwind to caller'
   // to the invoke destination.
----------------
Maybe just make this comment declarative: "This connects all the instructions which 'unwind to caller' to the invoke destination."

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:361-362
@@ +360,4 @@
+                                 TerminatePadArgs, TPI->getName(), TPI);
+        TPI->eraseFromParent();
+        UpdatePHINodes(&*BB);
+      }
----------------
We can take a page out of the SimplifyCFG book which does the same thing, and do something like this:
  Instruction *Replacement = nullptr;
  if (auto *TPI = dyn_cast<TerminatePadInst>(I)) ... / else if / ...
  if (Replacement) {
    Replacement->takeName(I);
    I->eraseFromParent();
    UpdatePHINodes(&*BB);
  }

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1105-1106
@@ +1104,4 @@
+    if (isFuncletEHPersonality(Personality)) {
+      DenseMap<BasicBlock *, ColorVector> CallerBlockColors =
+          colorEHFunclets(*Caller);
+      ColorVector &CallSiteColors = CallerBlockColors[OrigBB];
----------------
Hm, it seems pretty crummy that we have to recalculate this whole-function analysis for every call site that we want to consider inlining. This is definitely quadratic in basic blocks.

I think this hunk would actually be a good change to split out and do later. We can leave behind the noinline hack in clang for now.


http://reviews.llvm.org/D15139