This is an archive of the discontinued LLVM Phabricator instance.

Initial WebAssembly support in clang
ClosedPublic

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

Details

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish retitled this revision from to Initial WebAssembly support in clang.Aug 12 2015, 5:35 PM
sunfish updated this object.
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

@rengolin: FYI.

lib/Basic/Targets.cpp
6935

That's already the default?

6937

Already the default?

6938

TLSSupported = false for now.

6941

Not needed, since that's the default impl?

6956

I'm not sure we need this. Does it just make porting easier?

6983

final for these two.

6994

final

7015

final

lib/Driver/Tools.cpp
1559

New comment style without name -.

test/CodeGen/wasm-arguments.c
92

TODO for vararg test?

test/Driver/wasm32-unknown-unknown.cpp
52

Also test __wasm32__ (and 64 in the other file).

65

Test __REGISTER_PREFIX__ if we do keep it.

test/Preprocessor/init.c
8478

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

I thought I'd leave it in while we discuss whether this is really what we want.

6937

Right.

6938

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

We're going to put code in there before long anyway, so there's no harm in getting it ready.

6956

I don't know if we specifically need it; I just included it because lots of other popular targets have it.

6983

Right.

6994

This class is subclassed.

7015

This one is too.

lib/Driver/Tools.cpp
1559

Right.

test/CodeGen/wasm-arguments.c
92

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
52

test/Preprocessor/init.c covers this.

65

test/Preprocessor/init.c covers this.

test/Preprocessor/init.c
8478

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
6896

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
6899

True, leaving it as is sgtm.

6955

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

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
6955

WebAssembly32TargetInfo is subclassed in LLVM code, and getTargetDefines is overridden.

test/Preprocessor/init.c
8478

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
6947

Does int 128 get expanded in legalization? Seems good.

test/Preprocessor/init.c
8478

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
362

It's more common to have no spaces for these comments: /*UseARMMethodPtrABI=*/true,.

lib/Driver/Tools.cpp
1564

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
362

Ok. I updated this in my local patch.

lib/Driver/Tools.cpp
1564

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

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

Might make sense to copy the x86 bits here?

7633–7658

Seems overly complicated? Maybe just a positive test?

lib/CodeGen/CodeGenModule.cpp
824

Update the comment?

lib/Driver/ToolChains.cpp
3941–3961

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

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

Ok, I'll add a longer comment.

lib/Basic/Targets.cpp
6935–6941

The x86 bits don't handle "-" attributes, so it's not directly applicable.

7633–7658

Good idea. I'll do that.

lib/CodeGen/CodeGenModule.cpp
824

Ok, I'll write a bit more here.

lib/Driver/ToolChains.cpp
3941–3961

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

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

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

Why not just a positive test?

7643–7649

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

I do feel more comfortable rejecting anything that I'm not specifically expecting in this area.

7643–7649

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

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

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!