Page MenuHomePhabricator

[WebAssembly] memory.fill
ClosedPublic

Authored by tlively on Feb 5 2019, 2:52 PM.

Details

Summary

memset lowering, fix argument types in memcpy lowering, and
test encodings. Depends on D57736.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Feb 5 2019, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 2:52 PM
sbc100 added a comment.Feb 5 2019, 3:23 PM

Nice! I like the use of .s file for testing too!

llvm/test/MC/WebAssembly/bulk-memory-encodings.s
18 ↗(On Diff #185416)

I think this should just be end. I think end_* variants are supposed to be internal. Might want to check with wouter though.

Looks OK to me, some nits and questions:

llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td
36 ↗(On Diff #185416)

Nit: whitespace between = and 1

48 ↗(On Diff #185416)

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?

llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
57 ↗(On Diff #185416)

Why use getAnyExtOrTrunc for Val and getZExtOrTrunc for Size? What's the difference?

llvm/test/CodeGen/WebAssembly/bulk-memory-intrinsics.ll
15 ↗(On Diff #185416)

Nit: how about merging it in D57736?

llvm/test/CodeGen/WebAssembly/bulk-memory.ll
46 ↗(On Diff #185416)

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?

tlively marked 3 inline comments as done.Feb 11 2019, 10:25 AM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td
48 ↗(On Diff #185416)

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.

llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
57 ↗(On Diff #185416)

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.

llvm/test/CodeGen/WebAssembly/bulk-memory.ll
46 ↗(On Diff #185416)

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.

aheejin added inline comments.Feb 11 2019, 3:37 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrBulkMemory.td
48 ↗(On Diff #185416)

I think hasSideEffects is OK here because 1. It actually has side effects and 2. Given that data.drop is not gonna be in the middle of some user function, that wouldn't hinder optimization too much anyway.

llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
57 ↗(On Diff #185416)

I see. A bit of comments on that would also be better I think.

llvm/test/CodeGen/WebAssembly/bulk-memory.ll
46 ↗(On Diff #185416)

I see. But there seem to be other i8 val or len arguments without zeroext attribute in this file.

tlively retitled this revision from [WebAssembly] memory.fill and finish bulk memory to [WebAssembly] memory.fill.Feb 12 2019, 10:10 AM
tlively updated this revision to Diff 186518.Feb 12 2019, 10:57 AM
tlively marked 8 inline comments as done.
  • Address comments
llvm/lib/Target/WebAssembly/WebAssemblySelectionDAGInfo.cpp
57 ↗(On Diff #185416)

I forgot that llvm.memset always takes an i8 so this was already correct. I added a comment though.

llvm/test/CodeGen/WebAssembly/bulk-memory.ll
46 ↗(On Diff #185416)

Yes, it doesn't make a difference for i32 arguments so I just left it out.

llvm/test/MC/WebAssembly/bulk-memory-encodings.s
18 ↗(On Diff #185416)

I tried this out but the assembler didn't like it.

aheejin accepted this revision.Feb 12 2019, 5:53 PM
This revision is now accepted and ready to land.Feb 12 2019, 5:53 PM
Closed by commit rL353986: [WebAssembly] memory.fill (authored by tlively, committed by ). · Explain WhyFeb 13 2019, 2:25 PM
This revision was automatically updated to reflect the committed changes.