[PATCH] Extended support for native Windows C++ EH outlining
Reid Kleckner
rnk at google.com
Thu Mar 5 14:29:07 PST 2015
Some thoughts. I'll try to write up a more precise proposal for llvm.eh.actions.
REPOSITORY
rL LLVM
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:74-77
@@ -59,2 +73,6 @@
: FunctionPass(ID), DwarfPrepare(createDwarfEHPass(TM)) {}
+ ~WinEHPrepare() {
+ DeleteContainerSeconds(CatchHandlerMap);
+ DeleteContainerSeconds(CleanupHandlerMap);
+ }
----------------
You should do this and call .clear() at the end of runOnFunction(). My understanding is that FunctionPasses aren't recreated before and after each function.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:131-136
@@ +130,8 @@
+ const LandingPadInst *OriginLPad;
+ TinyPtrVector<const ExtractValueInst *> ExtractedEHPtrs;
+ TinyPtrVector<const ExtractValueInst *> ExtractedSelectors;
+ TinyPtrVector<const StoreInst *> EHPtrStores;
+ TinyPtrVector<const StoreInst *> SelectorStores;
+ TinyPtrVector<const Value *> EHPtrStoreAddrs;
+ TinyPtrVector<const Value *> SelectorStoreAddrs;
+};
----------------
Can you throw in a comment about how normally we only ever see at most one of these per landingpad?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:435
@@ +434,3 @@
+ // FIXME: We need a guard against partially outlined functions.
+ if (HandlersOutlined) {
+ // Insert a call to eh_actions
----------------
Maybe do:
if (!HandlersOutlined)
continue;
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:442-444
@@ +441,5 @@
+ for (ActionHandler *Action : Actions) {
+ // This is a placeholder for the EH context value of the action.
+ // FIXME: Add code somewhere to compute this value.
+ ActionArgs.push_back(ConstantInt::get(Type::getInt32Ty(Context), 0));
+ CastInst *HandlerPtr = CastInst::CreatePointerCast(
----------------
Would you say this was the "EH state" that we talked about, or something else?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:472-480
@@ +471,11 @@
+
+ // Remove all other instructions in this block.
+ BasicBlock::iterator II = Recover;
+ --II;
+ while (cast<Instruction>(II) != LPad) {
+ Instruction *Inst = II;
+ --II;
+ // This skips the handler bitcasts.
+ if (!(Inst->hasOneUse() && Inst->user_back() == Recover))
+ Inst->eraseFromParent();
+ }
----------------
What happens if these instructions still have uses? I think we'll assert on this code for example:
lpad:
%ehvals = landingpad
%x = load i32* %x.ptr
... ; normal EH dispatch
catch.int:
store i32 %x, i32* %x.ptr
I think a cleaner way to do this would be:
1. create a new basic block for the llvm.eh.actions call
2. assert that there are no phi nodes on the old landingpad block (EH preparation should've demoted them)
3. clone the LandingPadInst from the old one into the new one
4. set the unwind edge of all incoming invokes to the new landingpad
5. old landingpad is now trivially unreachable, allow removeUnreachableBlocks to do its thing
The advantage of this approach is that you never have to try to update the def-use graph.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:607-618
@@ -428,2 +606,14 @@
Instruction *ParentInst = cast<Instruction>(ParentVal);
- ParentAlloca = DemoteRegToStack(*ParentInst, true, ParentInst);
+ // FIXME: This is a work-around to temporarily handle the case where an
+ // instruction that is only used in handlers is not sunk.
+ // Without uses, DemoteRegToStack would just eliminate the value.
+ // This will fail if ParentInst is an invoke.
+ if (ParentInst->getNumUses() == 0) {
+ BasicBlock::iterator InsertPt = ParentInst;
+ ++InsertPt;
+ ParentAlloca =
+ new AllocaInst(ParentInst->getType(), nullptr,
+ ParentInst->getName() + ".reg2mem", InsertPt);
+ new StoreInst(ParentInst, ParentAlloca, InsertPt);
+ } else {
+ ParentAlloca = DemoteRegToStack(*ParentInst, true, ParentInst);
----------------
Can you help me understand this case? Is it something like this?
```
void f() {
HasCleanup o;
int x = g(); // invoke value used in catch
try {
g();
} catch (int) {
use_x(x);
}
}
```
I wonder if this is a bug in DemoteRegToStack that can be shown with opt -reg2mem.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:667
@@ +666,3 @@
+// WinEHCleanupDirector::handleTypeIdFor().
+static bool isSelectorDispatch(BasicBlock *BB, BasicBlock *&CatchHandler,
+ Constant *&Selector, BasicBlock *&NextBB) {
----------------
Can this be simplified with PatternMatch::match()?
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:770-772
@@ -535,3 +769,5 @@
- BasicBlock::iterator II = LPad;
+ BasicBlock::iterator II = StartBB->begin();
+ if (cast<Instruction>(II) == LPad)
+ ++II;
----------------
You might want StartBB->getFirstInsertionPt(). Also, consider this test case:
```
void use_it(int);
void g();
void f() {
int g_calls = 0;
try {
g(); g_calls++;
g(); g_calls++;
g(); g_calls++;
} catch (int) {
use_it(g_calls);
}
}
```
Using -O2, we get this interesting landingpad:
```
lpad: ; preds = %invoke.cont1, %invoke.cont, %entry
%g_calls.0 = phi i32 [ 2, %invoke.cont1 ], [ 1, %invoke.cont ], [ 0, %entry ]
%1 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
catch i8* bitcast (%rtti.TypeDescriptor2* @"\01??_R0H at 8" to i8*)
```
We need to make sure that phi gets demoted. We can probably do something like `assert(StartBB->begin() == StartBB()->getFirstNonPhi());`
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:882-889
@@ +881,10 @@
+ if (LoadAddr == StoreAddr) {
+ // Handle the common debug scenario where this loaded value is stored
+ // to a different location.
+ for (auto *U : Load->users()) {
+ if (auto *Store = dyn_cast<StoreInst>(U)) {
+ EHPtrStores.push_back(Store);
+ EHPtrStoreAddrs.push_back(Store->getPointerOperand());
+ }
+ }
+ return true;
----------------
This seems like an unexpected side effect for a function called "isEHPtrLoad".
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:896
@@ -596,3 +895,3 @@
- // Any other store just gets cloned.
- return CloningDirector::CloneInstruction;
+bool LandingPadMapper::isSelectorLoad(const LoadInst *Load) {
+ // This makes the assumption that a store we've previously seen dominates
----------------
These two functions could be refactored to be parameterized over the vector of stores, either EHPtrStores of SelectorStores.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:934-937
@@ -602,8 +933,6 @@
// Look for loads of (previously suppressed) landingpad values.
- // The EHPtr load can be ignored (it should only be used as
- // an argument to llvm.eh.begincatch), but the selector value
- // needs to be mapped to a constant value of 1 to be used to
- // simplify the branching to always flow to the current handler.
- const Value *LoadAddr = Load->getPointerOperand();
- if (LoadAddr == EHPtrStoreAddr) {
- VMap[Inst] = UndefValue::get(Int8PtrType);
+ // The EHPtr load can be mapped to an undef value as it should only be used
+ // as an argument to llvm.eh.begincatch, but the selector value needs to be
+ // mapped to a constant value of 1. This value will be used to simplify the
+ // branching to always flow to the current handler.
+ if (LPadMapper.isSelectorLoad(Load)) {
----------------
It's kind of amusing that the ehptr hasn't been used for anything yet... I wonder if there's any way to get MSVC to use the second argument to the catch handler.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1061
@@ +1060,3 @@
+// process.
+static bool isLandingPadEmptyResume(const BasicBlock *LPadBB) {
+ assert(LPadBB->isLandingPad());
----------------
I still don't think we need this. I'd rather hold off on committing this and experiment with making the llvm.eh.endcatch() cleanup non-exceptional.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1218
@@ +1217,3 @@
+ ValueToValueMapTy &VMap, const InvokeInst *Invoke, BasicBlock *NewBB) {
+ // All invokes in cleanup handlers can be replaced with calls.
+ SmallVector<Value *, 16> CallArgs(Invoke->op_begin(), Invoke->op_end() - 3);
----------------
I'm OK doing this for now, this isn't true in general, so we should put a FIXME here. Consider something horrible like this:
```
void might_throw();
struct MyDtor {
__forceinline ~MyDtor() {
try { might_throw(); } catch (int) {}
}
};
void f() {
MyDtor o;
might_throw();
}
```
After inlining, f will have a cleanup with an invoke in it.
================
Comment at: test/CodeGen/WinEH/cppeh-inalloca.ll:73
@@ +72,3 @@
+; CHECK: catch i8* bitcast (%rtti.TypeDescriptor2* @"\01??_R0H at 8" to i8*)
+; CHECK-NOT: %2 = extractvalue { i8*, i32 } %1, 0
+; CHECK-NOT: store i8* %2, i8** %exn.slot
----------------
Negative checks should generally try to be overly general, since small changes in the output can easily leave behind an extractvalue instruction that isn't named '%2', for example. In this case, I'd go for:
; CHECK-NOT: extractvalue { i8*, i32 }
; CHECK-NOT: br label
This basically says, there shouldn't be extractvalue or branches before we hit the llvm.eh.actions call.
================
Comment at: test/CodeGen/WinEH/cppeh-nested-2.ll:50-51
@@ +49,4 @@
+; CHECK: %outer = getelementptr inbounds %struct._Z4testv.ehdata, %struct._Z4testv.ehdata* %eh.data, i32 0, i32 4
+; (FIXME -- This should be eliminated, but it isn't yet.) %exn.slot = alloca i8*
+; (FIXME -- This should be eliminated, but it isn't yet.) %ehselector.slot = alloca i32
+; CHECK-NOT: %inner = alloca %class.Inner, align 1
----------------
Should be gone as of this morning
http://reviews.llvm.org/D7886
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list