This patch adds initial WebAssembly support in clang. The WebAssembly target is currently experimental.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/clang/Basic/TargetCXXABI.h | ||
---|---|---|
166 ↗ | (On Diff #32009) | @rengolin: FYI. |
lib/Basic/Targets.cpp | ||
6935 ↗ | (On Diff #32009) | That's already the default? |
6937 ↗ | (On Diff #32009) | Already the default? |
6938 ↗ | (On Diff #32009) | TLSSupported = false for now. |
6941 ↗ | (On Diff #32009) | Not needed, since that's the default impl? |
6956 ↗ | (On Diff #32009) | I'm not sure we need this. Does it just make porting easier? |
6983 ↗ | (On Diff #32009) | final for these two. |
6994 ↗ | (On Diff #32009) | final |
7015 ↗ | (On Diff #32009) | final |
lib/Driver/Tools.cpp | ||
1559 ↗ | (On Diff #32009) | New comment style without name -. |
test/CodeGen/wasm-arguments.c | ||
91 ↗ | (On Diff #32009) | TODO for vararg test? |
test/Driver/wasm32-unknown-unknown.cpp | ||
51 ↗ | (On Diff #32009) | Also test __wasm32__ (and 64 in the other file). |
64 ↗ | (On Diff #32009) | Test __REGISTER_PREFIX__ if we do keep it. |
test/Preprocessor/init.c | ||
8478 ↗ | (On Diff #32009) | ATOMIC_*_LOCK_FREE should always be 1 for "maybe". |
lib/Basic/Targets.cpp | ||
---|---|---|
6935 ↗ | (On Diff #32009) | I thought I'd leave it in while we discuss whether this is really what we want. |
6937 ↗ | (On Diff #32009) | Right. |
6938 ↗ | (On Diff #32009) | Since the MVP doesn't have threads at all, all variables are TLS :-). And after the MVP, we'll add threads with TLS. Is there a downside to leaving this on for now? It'll make the thread prototyping work easier. |
6941 ↗ | (On Diff #32009) | We're going to put code in there before long anyway, so there's no harm in getting it ready. |
6956 ↗ | (On Diff #32009) | I don't know if we specifically need it; I just included it because lots of other popular targets have it. |
6983 ↗ | (On Diff #32009) | Right. |
6994 ↗ | (On Diff #32009) | This class is subclassed. |
7015 ↗ | (On Diff #32009) | This one is too. |
lib/Driver/Tools.cpp | ||
1559 ↗ | (On Diff #32009) | Right. |
test/CodeGen/wasm-arguments.c | ||
91 ↗ | (On Diff #32009) | There's a TODO for "implement va_list properly" elsewhere, so we can add tests when we actually implement it. |
test/Driver/wasm32-unknown-unknown.cpp | ||
51 ↗ | (On Diff #32009) | test/Preprocessor/init.c covers this. |
64 ↗ | (On Diff #32009) | test/Preprocessor/init.c covers this. |
test/Preprocessor/init.c | ||
8478 ↗ | (On Diff #32009) | Clang is setting these automatically based on the value of MaxAtomicInlineWidth. Are you saying 32 is the wrong value for wasm32? This is perhaps a discussion to be had on the spec side. The value for wasm64 is also an interesting topic for the spec side. |
- use more default values in WebAssemblyTargetInfo etc.
- add a comment about using ARM-C++-ABI-style guard variables (for now)
- fix diff to include context for Phabricator
lib/Basic/Targets.cpp | ||
---|---|---|
6935 ↗ | (On Diff #32109) | I've now removed this (for now; we can discuss what to do in a later patch). |
lib/Basic/Targets.cpp | ||
---|---|---|
6938 ↗ | (On Diff #32109) | True, leaving it as is sgtm. |
6994 ↗ | (On Diff #32109) | Weird, the comment moved around. I put it on WebAssembly32TargetInfo which doesn't seem subclassed? Same for 64. Or is that going to be on the LLVM side? final on getTargetDefines instead? |
test/Preprocessor/init.c | ||
8478 ↗ | (On Diff #32109) | Yes, we don't know yet whether we guarantee atomicity so returning "maybe" is the conservative thing (which we can change in the future). We can discuss what we guarantee on the spec side, but for now the conservative thing is better IMO (and changing it isn't a problem, whereas changing "always" is a problem). FWIW N4509 is relevant. |
- set MaxAtomicInlineWidth to 0 for now (sets *_ATOMIC_*_LOCK_FREE to 1)
- enable LargeArrayAlign
- enable __int128
- various cleanups
lib/Basic/Targets.cpp | ||
---|---|---|
7010 ↗ | (On Diff #32192) | WebAssembly32TargetInfo is subclassed in LLVM code, and getTargetDefines is overridden. |
test/Preprocessor/init.c | ||
8478 ↗ | (On Diff #32192) | Ok, I've set MaxAtomicInlineWidth to 0 for now so that we don't block the rest of the patch on this issue. N4509 is just about exposing the same information via a different interface. |
The patch evolved enough to prompt posting one more new version; major changes:
- make constructors and destructors return this
- enable -fuse-init-array
- enable -fno-common
- disable -mdisable-fp-elim
- put static init code in ".text.__startup"
- define a "+simd128" feature, -msimd128 option, wasm_simd128 macro
- ignore empty struct arguments and return values
- more tests
Still lgtm, with minor comments.
lib/CodeGen/ItaniumCXXABI.cpp | ||
---|---|---|
364 ↗ | (On Diff #32485) | It's more common to have no spaces for these comments: /*UseARMMethodPtrABI=*/true,. |
lib/Driver/Tools.cpp | ||
1567 ↗ | (On Diff #32485) | Could you expand a bit on why "native" doesn't make sense if LLVM itself wasn't compiled for wasm? |
Minor updates to keep the patch building and working as code changes around it. Also enabled -mthread-model.
Some inline comments.
Thanks!
-eric
include/clang/Basic/TargetCXXABI.h | ||
---|---|---|
166 ↗ | (On Diff #33611) | Can you elaborate on the comment here as to what the alignment here means or something? It looks incomplete otherwise. |
lib/Basic/Targets.cpp | ||
6935–6941 ↗ | (On Diff #33611) | Might make sense to copy the x86 bits here? |
7633–7658 ↗ | (On Diff #33611) | Seems overly complicated? Maybe just a positive test? |
lib/CodeGen/CodeGenModule.cpp | ||
824 ↗ | (On Diff #33611) | Update the comment? |
lib/Driver/ToolChains.cpp | ||
3853–3873 ↗ | (On Diff #33611) | No generic defaults here? Might also make sense to have these all inline if they're just return true/return false. |
lib/Driver/Tools.cpp | ||
1564–1569 ↗ | (On Diff #33611) | I really dislike the idea of an ifdef here for this behavior. Can you explain some more? :) |
Thanks for the review!
include/clang/Basic/TargetCXXABI.h | ||
---|---|---|
166 ↗ | (On Diff #33611) | Ok, I'll add a longer comment. |
lib/Basic/Targets.cpp | ||
6935–6941 ↗ | (On Diff #33611) | The x86 bits don't handle "-" attributes, so it's not directly applicable. |
7633–7658 ↗ | (On Diff #33611) | Good idea. I'll do that. |
lib/CodeGen/CodeGenModule.cpp | ||
824 ↗ | (On Diff #33611) | Ok, I'll write a bit more here. |
lib/Driver/ToolChains.cpp | ||
3853–3873 ↗ | (On Diff #33611) | All these functions are returning non-default values or are overriding pure virtual functions. And they're outline because they're virtual overrides, so there's little optimization advantage to defining them in the header. And it's what most of the other toolchain definitions do most of the time. |
lib/Driver/Tools.cpp | ||
1564–1569 ↗ | (On Diff #33611) | As we discussed on IRC, I'll remove this code for now. |
Updated to address review comments:
- added comments
- simplified Triple validation code
- removed -mcpu="native" code
A couple things inline that need changing, but feel free to commit after without a repost.
-eric
lib/Basic/Targets.cpp | ||
---|---|---|
6943–6944 ↗ | (On Diff #33637) | The backend should handle any weirdness here with missing features especially as this will report an error based on -cc1 compilation and not the main command line. I.e. it's not necessary, that said if you feel wedded to it there's no problem either. |
7635–7639 ↗ | (On Diff #33637) | Why not just a positive test? |
7643–7649 ↗ | (On Diff #33637) | Ditto. (I said this just below, but it seems to have gotten munged in the newer version) |
lib/Basic/Targets.cpp | ||
---|---|---|
6943–6944 ↗ | (On Diff #33637) | I do feel more comfortable rejecting anything that I'm not specifically expecting in this area. |
7643–7649 ↗ | (On Diff #33637) | I actually did see your comment and updated the code accordingly. It now does a positive test, Triple == llvm::Triple("wasm64-unknown-unknown"), which is simpler than what it did before. However, it's also doing additional tests, because the Triple class's operator== doesn't distinguish between an Unknown that was actually "unknown" or an unknown that was some other string. Until we figure out what "vendor", "OS", and "environment" variations of wasm make sense (if any), we want to avoid dealing with accidental alternate triples. |
lib/Basic/Targets.cpp | ||
---|---|---|
7643–7649 ↗ | (On Diff #33637) | I think this is a lot of overkill here, no other target cares about this so why should the wasm target? If it's that important maybe fix Triple? |
lib/Basic/Targets.cpp | ||
---|---|---|
7643–7649 ↗ | (On Diff #33637) | This Triple issue is not important enough to hold up the rest of the patch for, so I just removed it. Thanks for the review! |