[PATCH] Extended support for native Windows C++ EH outlining
Andy Kaylor
andrew.kaylor at intel.com
Thu Mar 5 16:46:55 PST 2015
I'll rebase to pull in your latest changes and clean this up as you've suggested.
So, do you think it's worth pushing this forward on this review, or would you rather I peel off a piece or two?
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);
+ }
----------------
rnk wrote:
> 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.
Yeah, I thought I had a reason for keeping the actions around, but after thinking about it some more, that isn't necessary.
================
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;
+};
----------------
rnk wrote:
> Can you throw in a comment about how normally we only ever see at most one of these per landingpad?
Sure.
================
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
----------------
rnk wrote:
> Maybe do:
> if (!HandlersOutlined)
> continue;
OK
================
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(
----------------
rnk wrote:
> Would you say this was the "EH state" that we talked about, or something else?
Yes, this is EH state. I'll fix the name in the comment here.
================
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();
+ }
----------------
rnk wrote:
> 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.
Sounds good.
================
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);
----------------
rnk wrote:
> 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.
No, case you cite here doesn't do this.
The case this is meant to handle shows up in the cppeh-nonalloca-frame-values.ll test. Basically, any time a temporary value is computed in the normal code but only referenced in the catch code this happens. The source code for that test looks like this:
```
void test() {
int NumExceptions = 0;
int ExceptionVal[10];
SomeData Data = { 0, 0 };
for (int i = 0; i < 10; ++i) {
try {
may_throw();
Data.a += i;
}
catch (int e) {
ExceptionVal[NumExceptions] = e;
++NumExceptions;
if (e == i)
Data.b += e;
else
Data.a += e;
}
does_not_throw(NumExceptions);
}
dump(ExceptionVal, NumExceptions, Data);
}
```
The when this is compiled with optimization enabled, both i32 members of Data get initialized with a single i64 store. There's a GEP instruction in the entry block that gets the address of the 'b' memebr as %b, but %b is only used in the catch handling code.
What DemoteRegToStack is doing seems correct to me. If the value to be demoted has no uses then it just erases it and returns NULL. The trouble is caused by the fact that I'm doing this demotion after the catch handler code has been pruned. The only reference to the value was in the catch handler and the outlined handler is using a temporary alloca which is at this point waiting to be replaced by a framerecover call.
I think the right solution in this case is to sink the "computed" value into the catch handler. That could get a bit tricky in cases (probably very common) where values needed to by the value to be sunk aren't otherwise referenced in the handler, but I expect we can find a reasonable way to do it.
For instance in the case cited above, %b looks like this:
```
%b = getelementptr inbounds %struct.SomeData, %struct.SomeData* %tmpcast, i64 0, i32 1
```
Nothing in the catch handler references %tmpcast, so we'd need to pull it in. Right now we're creating a temporary alloca for %b in the value materializer and translating that to a framerecover call. I think if we recognize that %b is a non-alloca with no uses outside the catch handlers (I see this having the potential to go south here, but it would be easy enough for this case), then the materializer can create a temporary alloca for %tmpcast instead (if that hasn't been done already) and clone the %b instruction.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:667
@@ +666,3 @@
+// WinEHCleanupDirector::handleTypeIdFor().
+static bool isSelectorDispatch(BasicBlock *BB, BasicBlock *&CatchHandler,
+ Constant *&Selector, BasicBlock *&NextBB) {
----------------
rnk wrote:
> Can this be simplified with PatternMatch::match()?
I'll look into that.
================
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;
----------------
rnk wrote:
> This seems like an unexpected side effect for a function called "isEHPtrLoad".
Yeah, I should probably rename it. I'm pretty sure I've seen this kind of thing happening often in code generated with optimizations disabled.
================
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
----------------
rnk wrote:
> These two functions could be refactored to be parameterized over the vector of stores, either EHPtrStores of SelectorStores.
OK
================
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)) {
----------------
rnk wrote:
> 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.
Hard to say. It looks like it's just there to help out the debugger.
================
Comment at: lib/CodeGen/WinEHPrepare.cpp:1061
@@ +1060,3 @@
+// process.
+static bool isLandingPadEmptyResume(const BasicBlock *LPadBB) {
+ assert(LPadBB->isLandingPad());
----------------
rnk wrote:
> 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.
I'm fine with holding it back for now.
================
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);
----------------
rnk wrote:
> 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.
I believe the MS runtime will terminate the process if an exception occurs in a cleanup handler. So I think we'd need to ignore the __forceinline attribute in that case.
================
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
----------------
rnk wrote:
> 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.
Yeah, I was thinking about this yesterday as I was having to renumber temporary variables in some of the positive checks. These tests are probably going to be very brittle the way I've written them. At some point I want to go through and generalize them in a lot of ways. For now, I can certainly trim down the negative checks as you suggest.
================
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
----------------
rnk wrote:
> Should be gone as of this morning
Woo hoo!
http://reviews.llvm.org/D7886
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list