This is an archive of the discontinued LLVM Phabricator instance.

[WASM] SIMD128 support.
ClosedPublic

Authored by jpp on Jul 22 2016, 9:20 AM.

Details

Summary

Kicks off the implementation of wasm SIMD128 support (spec: https://github.com/stoklund/portable-simd/blob/master/portable-simd.md), adding support for add, sub, mul for i8x16, i16x8, i32x4, and f32x4.

The spec is WIP, and might change in the near future.

Diff Detail

Repository
rL LLVM

Event Timeline

jpp updated this revision to Diff 65089.Jul 22 2016, 9:20 AM
jpp retitled this revision from to Initial SIMD128 support for Wasm..
jpp updated this object.
jpp added a reviewer: dschuff.
jpp set the repository for this revision to rL LLVM.
jpp retitled this revision from Initial SIMD128 support for Wasm. to [WASM] SIMD128 support..Jul 22 2016, 11:46 AM
jpp updated this object.
dschuff added inline comments.Jul 26 2016, 8:54 AM
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
103 ↗(On Diff #65089)

XXX Do we want to make this helper unconditionally return the SIMD type and put the Subtarget check elsewhere?

lib/Target/WebAssembly/WebAssemblyInstrCall.td
38 ↗(On Diff #65089)

Should the predicate go outside the multiclass?

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
15 ↗(On Diff #65089)

Actually, I'm a bit unclear about Predicates [HasSIMD128] vs Requires<[HasSIMD128]>. Why do we need both here?

lib/Target/WebAssembly/WebAssemblyRegNumbering.cpp
78 ↗(On Diff #65089)

Should this be an assert instead?

sunfish added inline comments.Jul 26 2016, 9:35 AM
lib/Target/WebAssembly/WebAssemblySubtarget.cpp
69 ↗(On Diff #65089)

It's not necessary to check HasSIMD128 here, because we shouldn't have created an ARGUMENT_v16i8 etc. if we don't have HasSIMD128 in the first place :-). And without that check, this function doesn't need to be a member of WebAssemblySubtarget.

The same applies to other hasSIMD128() checks in the patch as well.

jpp added inline comments.Jul 26 2016, 9:59 AM
lib/Target/WebAssembly/WebAssemblySubtarget.cpp
69 ↗(On Diff #65089)

I was debating whether I should add all these hasSIMD128() checks everywhere or not, and then I decided to be pedantic. Should I remove them all?

sunfish added inline comments.Jul 26 2016, 11:16 AM
lib/Target/WebAssembly/WebAssemblySubtarget.cpp
69 ↗(On Diff #65089)

I favor removing them. We have to think about the subtarget restrictions when creating new instructions, but it's nice for code that's just reading instructions to just handle all of wasm.

jpp marked 3 inline comments as done.Jul 26 2016, 12:33 PM
jpp added inline comments.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
103 ↗(On Diff #65089)

The new patch does not check for hasSIMD128 in non-instruction-selection related files.

lib/Target/WebAssembly/WebAssemblyInstrCall.td
38 ↗(On Diff #65089)

I changed the SIMD instructions to be SIMD_I instead of plain I, and SIM_I Requires<[HasSIMD128]>;

lib/Target/WebAssembly/WebAssemblyRegNumbering.cpp
78 ↗(On Diff #65089)

not relevant anymore.

jpp updated this revision to Diff 65576.Jul 26 2016, 12:49 PM
jpp removed rL LLVM as the repository for this revision.

Addresses the comments.

jpp set the repository for this revision to rL LLVM.Jul 26 2016, 12:51 PM
dschuff added inline comments.Jul 27 2016, 8:34 AM
lib/Target/WebAssembly/WebAssemblyInstrCall.td
40 ↗(On Diff #65576)

The indentation here (and line 45 and 70 and 107) looks funny.

lib/Target/WebAssembly/WebAssemblyInstrInfo.td
113 ↗(On Diff #65576)

Oh, one other thing that's a little unclear to me. Why do we need ARGUMENT_v16i8 et al instead of just ARGUMENT_V128? I guess it's because it corresponds to MVTs which can be vectors, instead of register classes? Register classes would seem to make more sense (and the other ARGUMENT multiclass is based on WebAssemblyRegClass?)

jpp marked an inline comment as done.Jul 27 2016, 2:18 PM
jpp added inline comments.
lib/Target/WebAssembly/WebAssemblyInstrInfo.td
113 ↗(On Diff #65576)

I tried doing that before, but it upsets tablegen

llvm-tblgen: /usr/local/google/home/jpp/work/llvm/llvm/utils/TableGen/CodeGenDAGPatterns.h:74: llvm::MVT::SimpleValueType llvm::EEVT::TypeSet::getConcrete() const: Assertion `isConcrete() && "Type isn't concrete yet"' failed.

As far as I can tell, the problem here is that the V128 register class accommodates multiple MVT, while the other register classes deal with single MVTs.

jpp updated this revision to Diff 65804.Jul 27 2016, 2:27 PM
jpp added inline comments.
lib/Target/WebAssembly/WebAssemblyInstrInfo.td
113 ↗(On Diff #65804)

If I use ARGUMENT<V128> instead of SIMD_ARGUMENT<V128>, then tablegen compains

In ARGUMENT_V128: Could not infer all types in pattern!

But this is at least not an assertion. :)

dschuff accepted this revision.Aug 1 2016, 11:08 AM
dschuff edited edge metadata.

OK, I think this can go in. We can try to optimize the fake ARGUMENT registers as we go. I tried a few random things but didn't find out anything interesting. Not sure if @sunfish has any ideas about that? (see comments in WebAssemblyInstrInfo.td)

This revision is now accepted and ready to land.Aug 1 2016, 11:08 AM
sunfish accepted this revision.Aug 1 2016, 4:41 PM
sunfish added a reviewer: sunfish.

LGTM.

lib/Target/WebAssembly/WebAssemblyInstrInfo.td
113 ↗(On Diff #65804)

I think your analysis of the problem here is correct. The code here looks reasonable.

lib/Target/WebAssembly/WebAssemblyPeephole.cpp
192 ↗(On Diff #65804)

These hasSIMD128() checks are also redundant, along the lines of my earlier comment.

This revision was automatically updated to reflect the committed changes.