[PATCH] D57498: [WebAssembly] memory.copy

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:248
+  if (Subtarget->hasBulkMemory()) {
+    // Using memory.copy is always better than using loads and stores
+    MaxStoresPerMemcpy = 0;
----------------
dschuff wrote:
> 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.
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp:31
+  return DAG.getNode(WebAssemblyISD::MEMORY_COPY, DL, MVT::Other, Chain, Op1,
+                     Op2, Op3);
+}
----------------
aheejin wrote:
> 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. 
After looking into it for a bit, I wasn't able to get this to work :\


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57498/new/

https://reviews.llvm.org/D57498