[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