This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] SIMD loads and stores
ClosedPublic

Authored by tlively on Aug 28 2018, 2:34 PM.

Details

Summary

Reuse the patterns from WebAssemblyInstrMemory.td.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Aug 28 2018, 2:34 PM
tlively added inline comments.Aug 28 2018, 2:57 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
310 ↗(On Diff #162958)

I'm not actually sure these are in the right place. It looks like this switch returns the log of the default alignment for each type? The spec proposal says, "the memory access size is 16 bytes which is also the natural alignment." Am I reading that correctly to mean that these cases should return 4, not 3?

dschuff added inline comments.Aug 28 2018, 3:02 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
310 ↗(On Diff #162958)

Yes, that's correct; I'm pretty sure the intention is that the alignment is 16 (4 here). This is a requirement/optimization for some (sub)architectures such as SSE2 (and maybe ARM? not sure), as well as some native C ABIs.

tlively updated this revision to Diff 162980.Aug 28 2018, 4:15 PM
  • Fix natural alignment of SIMD memory ops
aheejin added inline comments.Aug 28 2018, 4:51 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
264 ↗(On Diff #162980)

Do we have tests for these load/store offset patterns? Tests for scalars are here, and I think we need similar variation of tests for SIMDs too. We probably wouldn't need all of offset variation tests for all existing vector types though. Maybe offset tests for one SIMD type would be sufficient.

tlively updated this revision to Diff 163244.Aug 29 2018, 6:08 PM
  • Test all patterns from offset.ll
tlively marked an inline comment as done.Aug 29 2018, 6:09 PM
tlively added inline comments.
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
264 ↗(On Diff #162980)

I implemented the tests for all types for now, since the marginal cost to do so was small. If we value having a smaller test file more than having all the types covered, I can remove some of the tests.

tlively marked an inline comment as done.Aug 29 2018, 6:13 PM
aheejin added inline comments.Aug 29 2018, 6:18 PM
lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
264 ↗(On Diff #162980)

Yeah it looks like you wrote it once and auto-generated for all other types so, more tests wouldn't hurt. :)

test/CodeGen/WebAssembly/simd-memory.ll
1 ↗(On Diff #163244)

How about changing filename to offset-simd.ll or something, because it looks like other offset-testing tests follow the same convention.

tlively updated this revision to Diff 163403.Aug 30 2018, 1:52 PM
  • Rename test file
tlively marked an inline comment as done.Aug 30 2018, 1:52 PM
aheejin added inline comments.Aug 30 2018, 2:17 PM
test/CodeGen/WebAssembly/offset-simd.td
1 ↗(On Diff #163403)

Oh not .td but .ll

tlively updated this revision to Diff 163408.Aug 30 2018, 2:25 PM
  • Fix file name
tlively marked an inline comment as done.Aug 30 2018, 2:26 PM
tlively added inline comments.
test/CodeGen/WebAssembly/offset-simd.td
1 ↗(On Diff #163403)

Oops, fixed.

aheejin accepted this revision.Aug 30 2018, 2:26 PM
This revision is now accepted and ready to land.Aug 30 2018, 2:26 PM
This revision was automatically updated to reflect the committed changes.
tlively marked an inline comment as done.