This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] v128.load{8,16,32,64}_lane instructions
ClosedPublic

Authored by tlively on Oct 13 2020, 11:01 PM.

Details

Summary

Prototype the newly proposed load_lane instructions, as specified in
https://github.com/WebAssembly/simd/pull/350. Since these instructions are not
available to origin trial users on Chrome stable, make them opt-in by only
selecting them from intrinsics rather than normal ISel patterns. Since we only
need rough prototypes to measure performance right now, this commit does not
implement all the load and store patterns that would be necessary to make full
use of the offset immediate. However, the full suite of offset tests is included
to make it easy to track improvements in the future.

Since these are the first instructions to have a memarg immediate as well as an
additional immediate, the disassembler needed some additional hacks to be able
to parse them correctly. Making that code more principled is left as future
work.

Diff Detail

Event Timeline

tlively created this revision.Oct 13 2020, 11:01 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2020, 11:01 PM
tlively requested review of this revision.Oct 13 2020, 11:01 PM
aheejin accepted this revision.Oct 14 2020, 4:10 AM
aheejin added inline comments.
clang/include/clang/Basic/BuiltinsWebAssembly.def
189

U in the third argument means pure. Can we say loads are pure? (The same for existing __builtin_wasm_load32_zero and __builtin_wasm_load64_zero)

clang/lib/CodeGen/CGBuiltin.cpp
16772

Would it be better to follow the arguments order in the original proposal? The order in here is mem, v, and lane. (I don't know why loads need v though) The same for intrinsic and instruction.

llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
246
315
llvm/test/CodeGen/WebAssembly/simd-load-lane-offset.ll
5

Maybe add a comment on which patterns are currently supported and unsupported among this file?

This revision is now accepted and ready to land.Oct 14 2020, 4:10 AM
tlively added inline comments.Oct 14 2020, 9:42 AM
clang/include/clang/Basic/BuiltinsWebAssembly.def
189
tlively planned changes to this revision.Oct 14 2020, 9:43 AM
tlively added inline comments.
llvm/test/CodeGen/WebAssembly/simd-load-lane-offset.ll
5

Will do.

tlively updated this revision to Diff 298209.Oct 14 2020, 12:08 PM
tlively marked 2 inline comments as done.
  • Add missing vec argument to loads and fix argument order
  • Add comments and fix formatting
This revision is now accepted and ready to land.Oct 14 2020, 12:08 PM
tlively requested review of this revision.Oct 14 2020, 12:09 PM

The structure is the same, but I would appreciate a review of the diff since a lot of things had to change.

Do loads need the vector argument? The proposal has that but I'm wondering why... Can that possibly be an error from the author's side?

clang/include/clang/Basic/BuiltinsWebAssembly.def
189

Can't SIMD loads trap? (Normal loads can AFAIK, right?)

Do loads need the vector argument? The proposal has that but I'm wondering why... Can that possibly be an error from the author's side?

Oops, I meant to answer that before but I did not. This is not a mistake because the purpose of the load_lane instructions is to replace just one lane of a vector. The vector to be be modified needs to be taken as an argument.

tlively added inline comments.Oct 14 2020, 6:22 PM
clang/include/clang/Basic/BuiltinsWebAssembly.def
189

Hmm yes, good point. I suppose that counts as a side effect so neither these nor the load_zero builtins should be pure?

tlively updated this revision to Diff 298291.Oct 14 2020, 8:39 PM
  • Remove "Pure" from load builtins
aheejin accepted this revision.Oct 15 2020, 4:37 AM
This revision is now accepted and ready to land.Oct 15 2020, 4:37 AM
This revision was landed with ongoing or failed builds.Oct 15 2020, 8:33 AM
This revision was automatically updated to reflect the committed changes.