Page MenuHomePhabricator

Initial WebAssembly support in clang
ClosedPublic

Authored by sunfish on Aug 12 2015, 5:35 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sunfish updated this revision to Diff 32009.Aug 12 2015, 5:35 PM
sunfish set the repository for this revision to rL LLVM.
sunfish added subscribers: cfe-commits, jfb.
jfb added a subscriber: rengolin.Aug 12 2015, 6:15 PM
jfb added inline comments.
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".

jfb added a reviewer: jfb.Aug 12 2015, 6:16 PM
sunfish planned changes to this revision.Aug 12 2015, 8:17 PM
sunfish marked 3 inline comments as done.
sunfish added inline comments.
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.

sunfish updated this revision to Diff 32025.Aug 12 2015, 8:19 PM
sunfish marked 3 inline comments as done.

Minor changes to address review comments.

sunfish updated this revision to Diff 32109.Aug 13 2015, 4:40 PM
  • 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
sunfish marked 2 inline comments as done.Aug 13 2015, 4:55 PM
sunfish added inline comments.
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).

jfb added inline comments.Aug 14 2015, 1:55 PM
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.

sunfish updated this revision to Diff 32192.Aug 14 2015, 3:21 PM
sunfish marked an inline comment as done.
  • set MaxAtomicInlineWidth to 0 for now (sets *_ATOMIC_*_LOCK_FREE to 1)
  • enable LargeArrayAlign
  • enable __int128
  • various cleanups
sunfish added inline comments.Aug 14 2015, 3:27 PM
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.

jfb accepted this revision.Aug 14 2015, 6:05 PM
jfb edited edge metadata.

lgtm

lib/Basic/Targets.cpp
7002 ↗(On Diff #32192)

Does int 128 get expanded in legalization? Seems good.

test/Preprocessor/init.c
8478 ↗(On Diff #32192)

constexpr makes a world of difference I'm told :-)

This revision is now accepted and ready to land.Aug 14 2015, 6:05 PM
sunfish updated this revision to Diff 32485.Aug 18 2015, 6:19 PM
sunfish edited edge metadata.
sunfish marked an inline comment as done.

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
jfb added a comment.Aug 19 2015, 10:25 AM

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?

sunfish marked 2 inline comments as done.Aug 20 2015, 10:25 AM
sunfish added inline comments.
lib/CodeGen/ItaniumCXXABI.cpp
364 ↗(On Diff #32485)

Ok. I updated this in my local patch.

lib/Driver/Tools.cpp
1567 ↗(On Diff #32485)

Ok. I added a comment to my local patch that says: "native" isn't meaningful when cross compiling, so only support this when the host is also WebAssembly.

sunfish updated this revision to Diff 33611.Aug 31 2015, 12:32 PM
sunfish marked 2 inline comments as done.

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.

sunfish updated this revision to Diff 33637.Aug 31 2015, 3:15 PM

Updated to address review comments:

  • added comments
  • simplified Triple validation code
  • removed -mcpu="native" code
echristo accepted this revision.Sep 2 2015, 6:22 PM
echristo added a reviewer: echristo.

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)

sunfishcode added inline comments.
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.

echristo added inline comments.Sep 3 2015, 1:41 PM
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?

This revision was automatically updated to reflect the committed changes.
sunfishcode added inline comments.Sep 3 2015, 3:54 PM
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!