This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add unimplemented-simd128 subtarget feature
ClosedPublic

Authored by tlively on Jan 9 2019, 10:08 AM.

Details

Summary

This replaces the old ad-hoc -wasm-enable-unimplemented-simd
flag. Also makes the new unimplemented-simd128 feature imply the
simd128 feature.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Jan 9 2019, 10:08 AM
aheejin added a comment.EditedJan 9 2019, 11:11 AM

I thought the unimplemented SIMD thing was a temporary workaround. Are they not planned to be implemented in V8 for a long time so that we should make it as a feature?

lib/Target/WebAssembly/WebAssembly.td
32 ↗(On Diff #180870)

Didn't know we can specify dependences in features! Cool.

test/CodeGen/WebAssembly/simd-arith.ll
127 ↗(On Diff #180870)

Why are there changes in the test results in this file?

tlively marked an inline comment as done.Jan 9 2019, 11:28 AM
tlively added inline comments.
test/CodeGen/WebAssembly/simd-arith.ll
127 ↗(On Diff #180870)

Probably because the new implicit enabling of the sign-ext feature made sign_ext_inreg legal, which led to the explicit selection of extract_lane_s. Before extract_lane_s was being selected only because it was the default for an any_extend, which was then sign extended via the shifts.

tlively edited the summary of this revision. (Show Details)Jan 9 2019, 1:25 PM
tlively updated this revision to Diff 180915.Jan 9 2019, 1:26 PM
  • Decouple the simd128 and sign-ext features
tlively updated this revision to Diff 180917.Jan 9 2019, 1:28 PM
  • Put the sign-ext feature tablegen definition back in its original place
aheejin added a comment.EditedJan 9 2019, 1:41 PM

(We talked in person and decided not to make simd feature depend on signext)

Nice! Can we have a comment on the code why we are doing that? Basically what we've talked about - SIMD implementation needs signext_inreg for clean implementation but that doesn't mean we require signext feature to enable simd feature, so we are doing this... something like that

And I think we should revert the test changes now?

(We talked in person and decided not to make simd feature depend on signext)

Nice! Can we have a comment on the code why we are doing that? Basically what we've talked about - SIMD implementation needs signext_inreg for clean implementation but that doesn't mean we require signext feature to enable simd feature, so we are doing this... something like that

And I think we should revert the test changes now?

Good idea about the comment. The changes to the test should actually stay because the new test results are the better code that is produced when the sext_inreg node is matched on.

tlively updated this revision to Diff 180934.Jan 9 2019, 2:37 PM
  • Give more context in comment
aheejin added inline comments.Jan 9 2019, 2:58 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1108 ↗(On Diff #180934)

Hmm, the purpose of this comment is when other people who don't have any context of our discussions see this they would understand why it's done this way. I'm not sure if I can understand this if I'm not the one who had the discussion. At least I think we'd better to add that while simd128 feature does not requires signext feature's functionality, signext_inreg isel node allows us cleaner implementation of vector extract patterns in simd128, so we want to make use of that to implement the patterns even when signext feature is off.

tlively updated this revision to Diff 180941.Jan 9 2019, 3:08 PM
  • More comment
aheejin accepted this revision.Jan 9 2019, 3:11 PM
This revision is now accepted and ready to land.Jan 9 2019, 3:11 PM
This revision was automatically updated to reflect the committed changes.