This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Mask SIMD shift values
ClosedPublic

Authored by tlively on Jan 9 2019, 3:43 PM.

Details

Summary

V8 currently implements SIMD shifts as taking an immediate operation,
which disagrees with the spec proposal and the toolchain
implementation. As a stopgap measure to get things working, unroll all
vector shifts. For all unrolled shifts, new and old, mask the shift
values to preserve WebAssembly's shift semantics for large shifts.

Diff Detail

Event Timeline

tlively created this revision.Jan 9 2019, 3:43 PM
aheejin added inline comments.Jan 9 2019, 4:18 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1196

How is this different from just unrolling this as done in below, like

return DAG.UnrollVectorOp(Op.getNode());

?

tlively marked an inline comment as done.Jan 9 2019, 4:24 PM
tlively added inline comments.
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1196

For example (i8x16.shl (i8x16.splat (i32.const 1)) 8) should be equal to a vector of all 1 bytes, since it is shifted by the provided amount modulo the width of a lane. However, if you just naively unroll the shift, you end up with a series of shifts that look like (i32.shl (; lane value ;) 8)`, which shifts modulo 32 instead. The result is you get a vector of all zeroes instead of a vector of all 1 bytes. This code adds an AND instruction before unrolling to explicitly mask the shift amount into the correct range.

tlively planned changes to this revision.Jan 9 2019, 5:10 PM

Actually, I just realized the other unroll that was already there should also have this behavior. Fixing now.

tlively retitled this revision from [WebAssembly] Expand SIMD shifts while V8's implementation disagrees to [WebAssembly] Mask SIMD shift values.Jan 10 2019, 3:01 PM
tlively edited the summary of this revision. (Show Details)
tlively updated this revision to Diff 181168.Jan 10 2019, 3:02 PM
  • Unroll old shifts as well
aheejin added inline comments.Jan 11 2019, 3:16 PM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
1170

Minor thing: It's sometimes hard to remember which is operand 0 and which is 1.. can we have inline comment for some of arguments to DAG.getNode and DAG.UnrollVectorOp, something like D50277? (It is a not landed yet CL of mine, and I brought this just for an example, but I saw this kind of commenting many other places in LLVM too)

And also we can early-exit i32/i64 case by

// 32-bit and 64-bit unrolled shifts will have proper semantics
if (Op.getSimpleValueType().getVectorElementType().bitsGE(MVT::i32))
  return DAG.UnrollVectorOp(Op.getNode());

// Mask the operand to get proper semantics from 32-bit shift
SDValue Shift = Op.getOperand(1);
...
tlively updated this revision to Diff 181650.Jan 14 2019, 2:00 PM
  • Early return and inline comments
aheejin accepted this revision.Jan 14 2019, 5:04 PM
This revision is now accepted and ready to land.Jan 14 2019, 5:04 PM
This revision was automatically updated to reflect the committed changes.