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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 3 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td:48
 
-//===----------------------------------------------------------------------===//
-// data.drop
-//===----------------------------------------------------------------------===//
-
+let mayStore = 1 in
 defm DATA_DROP :
----------------
aheejin wrote:
> 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?
I wouldn't consider touching the system memory that stores segments to be `mayStore`, since the WebAssembly semantics cannot reason about that memory. The reason I marked this `mayStore` is to prevent it from being moved past `memory.init` instructions, which are able to observe the effects of `data.drop` by trapping on dropped segments. This is kind of using a large hammer to solve a small problem, but the only alternative I know of is 'hasSideEffects` which seems like an even bigger hammer.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp:57
+                     Dst, DAG.getAnyExtOrTrunc(Val, DL, MVT::i32),
+                     DAG.getZExtOrTrunc(Size, DL, MVT::i32));
+}
----------------
aheejin wrote:
> Why use `getAnyExtOrTrunc` for `Val` and `getZExtOrTrunc` for `Size`? What's the difference?
The semantics of memory.fill say that the value argument is taken to be the byte value that memory is filled with, so there is an implicit truncate in the operation itself. That means that it doesn't matter how the value is extended if it starts off at least as large as an i8. All the bits of the size argument are meaningful, though, so it would be bad if it were extended with anything except zeroes.

I suppose this is broken if `Val` is an i1, so I will fix that.


================
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)
----------------
aheejin wrote:
> 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?
Only i8 arguments have the zeroext attribute. Without it, the function does extra work to truncate and sign extend the argument, but I don't want that extra complexity in the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57791