[PATCH] D57791: [WebAssembly] memory.fill

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp:57
+                     Dst, DAG.getAnyExtOrTrunc(Val, DL, MVT::i32),
+                     DAG.getZExtOrTrunc(Size, DL, MVT::i32));
+}
----------------
aheejin wrote:
> tlively wrote:
> > 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.
> I see. A bit of comments on that would also be better I think.
I forgot that `llvm.memset` always takes an i8 so this was already correct. I added a comment though.


================
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:
> tlively wrote:
> > 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.
> I see. But there seem to be other i8 `val` or `len` arguments without `zeroext` attribute in this file.
Yes, it doesn't make a difference for i32 arguments so I just left it out.


================
Comment at: llvm/test/MC/WebAssembly/bulk-memory-encodings.s:18
+
+    end_function
----------------
sbc100 wrote:
> I think this should just be `end`.  I think  end_* variants are supposed to be internal.  Might want to check with wouter though.
I tried this out but the assembler didn't like it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57791