- User Since
- Jul 29 2016, 12:33 AM (134 w, 1 d)
- More work
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.
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.
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.
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
- Add successor relationship
Sorry, and thanks for fixing this.
Thu, Feb 21
Wed, Feb 20
Looks OK to me, we have all the info in MCSymbolWasm after all.
I think we don't need the map of <throwbb, ehpad> after all now each bb can have at most one EH pad successor. This was not true in the first proposal implementation, but is in the new proposal.
Tue, Feb 19
I think it'd be OK to relax this constraints, but I don't feel too strongly on either side though. Anyway LGTM to me.
Mon, Feb 18
- Delete unnecessary classes for notify patterns
Sun, Feb 17
- Use ATOMIC_NRI for the fence + test fix
Fri, Feb 15
Tue, Feb 12
Mon, Feb 11
If people have opinions on this final version, please let me know.
Looks OK to me, some nits and questions:
Sun, Feb 10
- Fix bug: 0 should be i32.const 0 and not an immediate
Fri, Feb 8
- Address comments
- Replace -matomics with -mthread-model posix in preprocessor test
I had an offline discussion with @tlively and @dschuff and decided to remove -atomics option in the driver. Instead, clang -cc1's -target-feature +atomics will be either of -pthread or -mthread-model posix is given. This is to reduce the total number of options and thus the total number of combinations of options. Sorry for frequent changes. Also addressed comments.
I only added those test routines in wasm-toolchain.c and not wasm-toolchain.cpp, because they are basically the same.
- Initialized ThreadModel vairables with ""
- Small fix
- Fix some bugs
- Make the driver error out when explicitly given options don't match, such as -no-pthread and -matomics are given at the same time
Thu, Feb 7
Sorry nevermind my previous code. There was not hacky and much cleaner way to do everything in the driver layer. (Before I tried to do everything in the cc1 compilation layer :( )
I don't think we can remove the threadmodel thing from the backend in near future (because atomics is a subtarget feature), so I suggest landing this because without this LTO wouldn't work.
Wed, Feb 6
- Small fix
Tue, Feb 5
- Fix typo
Thanks for the report. Disabled the failing line temporarily in rL353234 until it is fixed.