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

Heejin Ahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 11 09:03:00 PST 2019

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
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?

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list