This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Delete ThrowUnwindDest map from WasmEHFuncInfo
ClosedPublic

Authored by aheejin on Feb 20 2019, 6:28 PM.

Details

Summary

Before when we implemented the first EH proposal, 'catch <tag>'
instruction may not catch an exception so there were multiple EH pads an
exception can unwind to. That means a BB could have multiple EH pad
successors.

Now after we switched to the new proposal, every 'catch' instruction
catches an exception, and there is only one catchpad per catchswitch, so
we at most have one EH pad successor, making ThrowUnwindDest map in
WasmEHInfo unnecessary.

Keeping ThrowUnwindDest map in WasmEHInfo has its own problems,
because other optimization passes can split a BB that contains possibly
throwing calls (previously invokes), and we have to update the map every
time that happens, which is not easy for common CodeGen passes.

This also correctly updates successor info in LateEHPrepare when we add
a rethrow instruction.

Diff Detail

Event Timeline

aheejin created this revision.Feb 20 2019, 6:28 PM
aheejin planned changes to this revision.Feb 21 2019, 11:24 PM
aheejin updated this revision to Diff 187974.Feb 22 2019, 1:12 PM
  • Add successor relationship

It's hard to add a test for this because WasmEHFuncInfo is not serialized (All machine-specific function info is not serialized in mir or anywhere at the moment). But I wanted to split this from the other CL that uses this part because 1. that CL is already big 2. this is a bugfix

dschuff accepted this revision.Feb 22 2019, 1:34 PM

You mention that this is a bugfix. what's the bug that is fixed? An optimization not keeping the map up to date?

This revision is now accepted and ready to land.Feb 22 2019, 1:34 PM

Kind of. I found it was not updated properly in LateEHPrepare so I created D58474, and abandoned it because it's better not to use ThrowUnwindDestMap anyway, because that kind of BB splitting can occur in other non-wasm passes as well, and now we can use successor info instead because there is at most one EH pad successor for a BB. (This was not the case for the first proposal.)
But we cannot remove EHPadUnwindDest because it is used here to set the new successor for newly added rethrow instruction. And this info is I think safe from transformation from other passes because both of them are EH pads, and all passes I have seen refrain from doing optimization on EH pads. But maybe there will be some passes that does that.. I can't be 100% sure.

Oh and failure to update the newly created BB successor relationship by rethrow is a bug in itself, regardless of whether we use ThrowUnwindDestMap or not.

Makes sense.
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.

aheejin planned changes to this revision.Feb 22 2019, 4:32 PM

Oh turned out this (of course) changes an existing test because successor relationship was modified, which was also a pre-existing bug in the test. :( Will update the test soon.

aheejin updated this revision to Diff 188132.Feb 25 2019, 4:42 AM

Update test

This revision is now accepted and ready to land.Feb 25 2019, 4:42 AM
aheejin planned changes to this revision.Mar 1 2019, 1:04 PM
aheejin requested review of this revision.Mar 3 2019, 1:39 PM

Makes sense.
Is EHPadUnwindMap analogous to any of the similarly-named data structures for WinEH? If so, that would seem to make it even less likely to accidentally break, since any change that breaks it would also break WinEH.

Not exactly. (I tried but) I don't understand their structure 100% but it looks like they assign state numbers to layers of BB groups created by EH pads. So for example an EH pad has a state number 1, within BBs that's dominated by that EH pad, the next level EH pads are assigned number 2, and so on. But anyway, I think as long as this nested structure of EH pads are preserved we are ok.

(And I thought I had to change something but I didn't, so I'm changing the status back)

This revision is now accepted and ready to land.Mar 3 2019, 1:39 PM
aheejin edited the summary of this revision. (Show Details)Mar 3 2019, 2:34 PM
This revision was automatically updated to reflect the committed changes.