This is an archive of the discontinued LLVM Phabricator instance.

[X86] Optimize WinEH state stores
ClosedPublic

Authored by majnemer on Jan 31 2016, 9:33 PM.

Details

Summary

32-bit x86 Windows targets use a linked-list of nodes allocated on the
stack, referenced to via thread-local storage. The personality routine
interprets one of the fields in the node as a 'state number' which
indicates where the personality routine should transfer control.

State transitions are possible only before call-sites which may throw
exceptions. Our previous scheme had us update the state number before
all call-sites which may throw.

Instead, we can try to minimize the number of times we need to store by
reasoning about the nearest store which dominates the current call-site.
If the last store agrees with the current call-site, then we know that
the state-update is redundant and can be elided.

This is largely straightforward: a RPO walk of the blocks allows us to
correctly forward propagate the information when the function is a DAG.
Currently, loops are not handled optimally and may trigger superfluous
state stores.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 46508.Jan 31 2016, 9:33 PM
majnemer retitled this revision from to [X86] Optimize WinEH state stores.
majnemer updated this object.
majnemer added a reviewer: rnk.
majnemer added a subscriber: llvm-commits.
rnk added inline comments.Feb 2 2016, 1:27 PM
lib/Target/X86/X86WinEHState.cpp
422 ↗(On Diff #46508)

I'm not really a fan of all these long inline lambdas. I'd rather see this out of line, despite the paramter passing boilerplate:

static int getBaseStateForBB(... BBColors, ... FuncInfo, BasicBlock *BB) { ... }

You can practically guess the algorithm it's implementing from that prototype.

439 ↗(On Diff #46508)

ditto

466 ↗(On Diff #46508)

For SEH, I think we need state insertions prior to nounwind calls. This is the case I'm thinking of:

void f() {
  __try { crash(); }
  __except(1) { }
  printf("%s\n", nullptr);
}

The printf call will crash, and it should not end up being caught by the __except block. It needs a -1 store, despite being nounwind. You can avoid state stores prior to intrinsic calls in SEH though.

I guess this is a pre-existing bug.

490 ↗(On Diff #46508)

grumble grumble

test/CodeGen/WinEH/wineh-statenumbering.ll
137 ↗(On Diff #46508)

We should probably add a non-trivial case like:

void f(int cond) {
  if (cond) { __try { crash(); } __except(1) { } }
  g(); // need -1 store before g
}
majnemer updated this revision to Diff 48043.Feb 15 2016, 9:07 PM
  • Address review feedback
rnk accepted this revision.Feb 16 2016, 4:39 PM
rnk edited edge metadata.

lgtm, now our code won't be totally bone-headed. :)

This revision is now accepted and ready to land.Feb 16 2016, 4:39 PM
This revision was automatically updated to reflect the committed changes.