Page MenuHomePhabricator

[WebAssembly] Added 64-bit memory.grow/size/init/copy/fill
ClosedPublic

Authored by aardappel on Mon, Jun 29, 4:36 PM.

Details

Summary

This covers both the existing memory functions as well as the new bulk memory proposal.
Added new test files since changes where also required in the inputs.

Diff Detail

Event Timeline

aardappel created this revision.Mon, Jun 29, 4:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 29, 4:36 PM

@tlively I have no idea if my changes in IntrinsicsWebAssembly.td make sense, they merely pass the tests ;) please advise.

tlively added inline comments.Mon, Jun 29, 5:32 PM
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

I'm surprised this doesn't break the bulk-memory-intrinsics.ll test since this makes the intrinsic polymorphic. It would be good to add the 64-bit versions of these intrinsics to that test as well, and if the polymorphism starts causing problems, it would probably be a good idea to replace the second llvm_anyint_ty here with a LLVMMatchType<3> so you only have to specify a single type in the intrinsic name.

Alternatively, you could just delete these intrinsics and the corresponding clang builtins. They are extremely useless because there is no way to know what segment ID to pass before linking. I'm surprised we don't have intrinsics and builtins for memory.copy and memory.fill, which could actually be useful.

Another test you might want to update is the bulk-memory-encodings.s MC test.

dschuff added inline comments.Mon, Jun 29, 5:56 PM
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

do these segment IDs get rewritten by the linker, i.e. could they be symbolic? Are they only synthesized by the linker now or do we generate them in the backend? (I forget and I think we went back and forth on that). Would it make sense to have e.g. in the assembly language but not in C/LLVM IR?

tlively added inline comments.Mon, Jun 29, 9:38 PM
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

No, they cannot currently be symbolic. Right now they have to be constant integers. Even if they could be symbolic, we would have to change the linker to give symbols referenced that way their own segments, rather than merging them with all the other symbols into one big segment. If we made those changes, I could see these instructions being very useful in low-level assembly programs, though. The Binaryen segment optimizations are already robust enough that we wouldn't have to do any work in Binaryen for this feature.

dschuff added inline comments.Tue, Jun 30, 11:40 AM
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

ok thanks. seems worth keeping in mind, but obviously not a high priority unless we get a use case or request.

aardappel marked an inline comment as done.Thu, Jul 2, 1:43 PM
aardappel added inline comments.
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

Your suggestion of LLVMMatchType<3> gets me error GA8ED74C0: Parameter #4 has out of bounds matching number 3, and looking at the other definitions, it seems these refer to the previous list, not the current one?

aardappel marked an inline comment as done.Thu, Jul 2, 4:34 PM
aardappel added inline comments.
llvm/include/llvm/IR/IntrinsicsWebAssembly.td
209

Ok, tried actually fixing these intrinsics for wasm64, but ran into too much trouble, so taking @tlively up on his offer to remove them instead.

aardappel updated this revision to Diff 275258.Thu, Jul 2, 4:36 PM

Removed unused init/drop intrinsics instead of trying to make them work for wasm64.
Also testing bulk-memory-encoding.s

tlively added inline comments.Fri, Jul 3, 5:36 PM
llvm/test/MC/WebAssembly/bulk-memory-encodings.s
10

Getting rid of the intrinsics and clang builtins is fine, but I think we should keep the instruction definitions and these tests around. They would still be useful for disassembling modules that contain these instructions in the linker-synthesized memory initialization function.

aardappel updated this revision to Diff 275785.Mon, Jul 6, 12:07 PM

Reinstated init/drop defs + test

aardappel marked an inline comment as done.Mon, Jul 6, 12:07 PM
tlively accepted this revision.Mon, Jul 6, 12:23 PM

LGTM, thanks!

This revision is now accepted and ready to land.Mon, Jul 6, 12:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jul 6, 12:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript