This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] memory.copy
ClosedPublic

Authored by tlively on Jan 30 2019, 10:32 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Jan 30 2019, 10:32 PM
tlively updated this revision to Diff 184454.Jan 30 2019, 10:35 PM
  • Clean up pattern
dschuff added inline comments.Jan 31 2019, 1:20 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
248 ↗(On Diff #184454)

Is it smaller than even 1 store? Even if it is, stores can have constant offsets. Probably best to set this to at least 2.

aheejin added inline comments.Jan 31 2019, 3:34 PM
llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
31 ↗(On Diff #184454)

Do we need WebAssemblyISD::MEMORY_COPY node? Can we possibly lower it directly using DAG.getMachineNode here? In case that involves more work, nevermind. But if we are just transferring operands one-to-one that also looks as simple.

llvm/test/CodeGen/WebAssembly/bulk-memory.ll
66 ↗(On Diff #184454)

Are there any difference between memcpy_1, memory_4, and memory_1024 other than the argument to i32.const? Do we need all these three tests?

tlively updated this revision to Diff 184638.Jan 31 2019, 3:41 PM
  • Prefer to use single load-store pairs
tlively marked an inline comment as done.Feb 4 2019, 2:48 PM
tlively added inline comments.
llvm/test/CodeGen/WebAssembly/bulk-memory.ll
66 ↗(On Diff #184454)

No, we can probably get away with one test for the load-store pair and one for memory.copy.

tlively updated this revision to Diff 185188.Feb 4 2019, 4:38 PM
tlively marked 3 inline comments as done.
  • Address comments
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 4:38 PM
tlively added inline comments.Feb 4 2019, 4:38 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
248 ↗(On Diff #184454)

We agreed offline that setting this to 1 is fine for now (it was originally 0). Once bulk memory is correct throughout the toolchain, we will take measurements and figure out where these thresholds should be set.

llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
31 ↗(On Diff #184454)

After looking into it for a bit, I wasn't able to get this to work :\

aheejin accepted this revision.Feb 4 2019, 4:47 PM
This revision is now accepted and ready to land.Feb 4 2019, 4:47 PM
This revision was automatically updated to reflect the committed changes.