• More work

rebase

rebase

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.

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.

Thank you!

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.

• Delete unnecessary classes for notify patterns

• Use ATOMIC_NRI for the fence + test fix

If people have opinions on this final version, please let me know.

Looks OK to me, some nits and questions:

• Fix bug: 0 should be i32.const 0 and not an immediate

• Address comments

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

• 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.

In D57874#1391133, @tlively wrote:

In D57874#1389953, @aheejin wrote:

Anyway, moved all logic to the driver layer and did this:

• -matomics means -mthread-model posix
• -mthread-model posix means -matomics
• -pthread means both -matomics and -mthread-model posix

If you replace "-matomics" with "-mbulk-memory," I plan to duplicate the logic for items 2 and 3 above, but not 1. For bulk memory even more than atomics, there is a legitimate usecase for enabling it even in single threaded contexts (namely getting to use memory.copy and memory.fill). I wonder if consistency with how bulk memory works is a strong enough argument for dropping item 1 for atomics as well.

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 ""

In D57874#1389981, @sunfish wrote:

• -matomics means -mthread-model posix

The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code doesn't actually using threads.

• 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

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.

• Small fix

In D57798#1386470, @sbc100 wrote:

How does this relate to the existing -mattr=+atomics of llc? Will it also result in attributes #0 = { "target-features"="+atomics" } in the bitcode?

• Fix typo

Thanks for the report. Disabled the failing line temporarily in rL353234 until it is fixed.