[PATCH] Work-in-progress omnibus patch for native Windows C++ EH outlining

David Majnemer david.majnemer at gmail.com
Wed Feb 25 13:36:35 PST 2015


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:92
@@ +91,3 @@
+                                     BasicBlock *&NextBB,
+                                     SmallSet<BasicBlock *, 4> &VisitedBlocks);
+  BasicBlock *findCleanupCodeStart(BasicBlock *StartBB, BasicBlock *EndBB);
----------------
I'd typedef `SmallSet<BasicBlock *, 4>` in case we'd like to change it later.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:244-246
@@ +243,5 @@
+  ~LandingPadActions() {
+    for (ActionHandler *H : Actions) {
+      delete H;
+    }
+  }
----------------
This can just be `DeleteContainerPointers(Actions)`.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:257
@@ +256,3 @@
+
+  bool includesCleanup() { return HasCleanupHandlers; }
+
----------------
This can be a const method.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:259-260
@@ +258,4 @@
+
+  SmallVector<ActionHandler *, 4>::iterator begin() { return Actions.begin(); }
+  SmallVector<ActionHandler *, 4>::iterator end() { return Actions.end(); }
+
----------------
Can you use `SmallVectorImpl` instead of `SmallVector<ActionHandler *, 4>` here?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:499-500
@@ +498,4 @@
+      }
+    }
+    else {
+      // FIXME: Sink non-alloca values into the handler if they have no other
----------------
Please stick the else on the same line as the }

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:585-587
@@ +584,5 @@
+        ParentAlloca = DemoteRegToStack(*SI, true, SI);
+      } else if (auto *PN = dyn_cast<PHINode>(ParentVal))
+        ParentAlloca = DemotePHIToStack(PN, AllocaInsertPt);
+      else {
+        Instruction *ParentInst = dyn_cast<Instruction>(ParentVal);
----------------
Please consistently use braces here.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:588-589
@@ +587,4 @@
+      else {
+        Instruction *ParentInst = dyn_cast<Instruction>(ParentVal);
+        assert(ParentInst);
+        // FIXME: This is a work-around to temporarily handle the case where an
----------------
This can be simplified to just `auto *ParentInst = cast<Instruction>(ParentVal);`

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:600-601
@@ +599,4 @@
+          new StoreInst(ParentInst, ParentAlloca, InsertPt);
+        }
+        else {
+          ParentAlloca = DemoteRegToStack(*ParentInst, true, ParentInst);
----------------
Same line please.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:625
@@ -355,1 +624,3 @@
+      if (ParentAlloca == AllocaInsertPt)
+        AllocaInsertPt = dyn_cast<Instruction>(ElementPtr);
       delete ParentAlloca;
----------------
Is it ever OK for `AllocaInsertPt` to be null? If not, use `cast` instead of `dyn_cast`.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:705-714
@@ -390,1 +704,12 @@
+  Function *Handler;
+  if (CatchOrCleanup == Catch) {
+    FunctionType *FnType = FunctionType::get(Int8PtrType, ArgTys, false);
+    Handler = Function::Create(FnType, GlobalVariable::ExternalLinkage,
+                               SrcFn->getName() + ".catch", M);
+  } else {
+    FunctionType *FnType =
+        FunctionType::get(Type::getVoidTy(Context), ArgTys, false);
+    Handler = Function::Create(FnType, GlobalVariable::ExternalLinkage,
+                               SrcFn->getName() + ".cleanup", M);
+  }
 
----------------
I'm guessing we are not going to want to always give them `ExternalLinkage` but I guess we can fix that later.  Perhaps we should have a TODO?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:820-822
@@ +819,5 @@
+      for (User *EU : Extract->users()) {
+        const StoreInst *Store = dyn_cast<StoreInst>(EU);
+        // FIXME: Handle multiple stores of each value?
+        if (Store) {
+          if (Extract == ExtractedEHPtr) {
----------------
Might be a little easier to read as:
  if (!Store)
    continue;

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1029-1030
@@ +1028,4 @@
+      // to verify that this is our starting point.
+      const LandingPadInst *FoundLPad = dyn_cast<LandingPadInst>(&Inst);
+      if (FoundLPad)
+        continue;
----------------
This can simply be:
  if (isa<LandingPadInst>(&Inst))
    continue;

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1053-1054
@@ +1052,4 @@
+
+      const StoreInst *Store = dyn_cast<StoreInst>(&Inst);
+      if (Store) {
+        // Look for and suppress stores of the extracted landingpad values.
----------------
Please fold these two lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1071-1073
@@ +1070,5 @@
+      // There should be a call to llvm.eh.endcatch somewhere in the landing pad.
+      const IntrinsicInst *IntrinCall = dyn_cast<IntrinsicInst>(&Inst);
+      if (IntrinCall && IntrinCall->getIntrinsicID() == Intrinsic::eh_endcatch)
+        continue;
+
----------------
This can simply be:
  if (match(&Inst, m_Intrinsic<Intrinsic::eh_endcatch>()))
    continue;

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1075-1076
@@ +1074,4 @@
+
+      const LoadInst *Load = dyn_cast<LoadInst>(&Inst);
+      if (Load) {
+        // Look for loads of (previously suppressed) landingpad values.
----------------
Please fold these two lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1096-1097
@@ +1095,4 @@
+
+      const InsertValueInst *Insert = dyn_cast<InsertValueInst>(&Inst);
+      if (Insert) {
+        if (Insert->getInsertedValueOperand() == LoadedEHPtr ||
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1113-1114
@@ +1112,4 @@
+      // that scenario.
+      const BranchInst *Branch = dyn_cast<BranchInst>(&Inst);
+      if (Branch) {
+        if (!Branch->isUnconditional())
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1341-1342
@@ +1340,4 @@
+      // See if there is any interesting code executed before the catch.
+      BasicBlock *CleanupBB = findCleanupCodeStart(BB, BB);
+      if (CleanupBB) {
+        //   Add a cleanup entry to the list
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1361-1362
@@ +1360,4 @@
+    // See if there is any interesting code executed before the dispatch.
+    BasicBlock *CleanupBB = findCleanupCodeStart(BB, DispatchBB);
+    if (CleanupBB) {
+      //   Add a cleanup entry to the list
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1371-1372
@@ +1370,4 @@
+
+    BranchInst *Dispatch = dyn_cast<BranchInst>(DispatchBB->getTerminator());
+    assert(Dispatch);
+
----------------
You can use `cast` instead of `dyn_cast` here.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1383-1384
@@ +1382,4 @@
+  // See if there is any interesting code executed before the resume.
+  BasicBlock *CleanupBB = findCleanupCodeStart(BB, BB);
+  if (CleanupBB) {
+    //   Add a cleanup entry to the list
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1402-1406
@@ +1401,7 @@
+//
+BasicBlock *WinEHPrepare::findSelectorComparison(BasicBlock *BB, 
+                                          BasicBlock *&CatchHandler, 
+                                          Constant *&Selector,
+                                          BasicBlock *&NextBB,
+                                          SmallSet<BasicBlock *, 4> &VisitedBlocks) {
+  VisitedBlocks.insert(BB);
----------------
Please format this with clang-format.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1416
@@ +1415,3 @@
+
+    BasicBlock *DispatchBB = findSelectorComparison(Succ, CatchHandler, Selector, NextBB, VisitedBlocks);
+    if (DispatchBB)
----------------
Please format this with clang-format.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1452-1453
@@ +1451,4 @@
+    //   resume { i8*, i32 } %lpad.val2
+    ResumeInst *Resume = dyn_cast<ResumeInst>(Terminator);
+    if (Resume) {
+      InsertValueInst *Insert1 = nullptr;
----------------
Please fold these lines together.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1470
@@ +1469,3 @@
+          continue;
+        if (!Inst->hasOneUse() || (Inst->user_back() != Insert1 && Inst->user_back() != Insert2))
+          return BB;
----------------
Please reformat this.

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1500-1503
@@ +1499,6 @@
+        Compare = dyn_cast<CmpInst>(Branch->getCondition());
+      for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE; ++II) {
+        Instruction *Inst = II;
+        if (isa<PHINode>(Inst) || isa<DbgInfoIntrinsic>(Inst))
+          continue;
+        if (Inst == LPad || Inst == Compare || Inst == Branch)
----------------
Would this be simpler if you initialized your iterator to the instruction at `BB->getFirstNonPHIOrDbg()`?

================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1560-1561
@@ +1559,4 @@
+        continue;
+      }
+      else {
+        // Look for empty blocks with unconditional branches.
----------------
Same line please.

http://reviews.llvm.org/D7886

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






More information about the llvm-commits mailing list