[PATCH] D57791: [WebAssembly] memory.fill and finish bulk memory

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Looks OK to me, some nits and questions:



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td:36
 
-let mayStore = 1 in
+let mayLoad =1, mayStore = 1 in
 defm MEMORY_INIT :
----------------
Nit: whitespace between `=` and `1`


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td:48
 
-//===----------------------------------------------------------------------===//
-// data.drop
-//===----------------------------------------------------------------------===//
-
+let mayStore = 1 in
 defm DATA_DROP :
----------------
Why is it `mayStore`? I guess even it does not modify wasm linear memory it can possibly modify other parts of the memory that stores segments, is that right?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp:57
+                     Dst, DAG.getAnyExtOrTrunc(Val, DL, MVT::i32),
+                     DAG.getZExtOrTrunc(Size, DL, MVT::i32));
+}
----------------
Why use `getAnyExtOrTrunc` for `Val` and `getZExtOrTrunc` for `Size`? What's the difference?


================
Comment at: llvm/test/CodeGen/WebAssembly/bulk-memory-intrinsics.ll:15
   call void @llvm.wasm.memory.init.i32.i32.i8(
-    i32 0, i32 3, i8* %dest, i32 %offset, i32 %size
+    i32 3, i32 0, i8* %dest, i32 %offset, i32 %size
   )
----------------
Nit: how about merging it in D57736?


================
Comment at: llvm/test/CodeGen/WebAssembly/bulk-memory.ll:46
 ; BULK-MEM-NEXT: return
-declare void @llvm.memmove.p0i8.p0i8.i32(
-  i8* %dest, i8* %src, i32 %len, i1 %volatile
-)
-define void @memmove_i8(i8* %dest, i8* %src, i32 %len) {
-  call void @llvm.memmove.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 0)
+define void @memset_i8(i8* %dest, i8 zeroext %val, i8 zeroext %len) {
+  call void @llvm.memset.p0i8.i8(i8* %dest, i8 %val, i8 %len, i1 0)
----------------
In this file, some `val` and `len` arguments have `zeroext` attribute and some don't. Is there a reason? And do why we need this attribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57791