[PATCH] D82821: [WebAssembly] Added 64-bit memory.grow/size/init/copy/fill

Mon Jun 29 17:57:30 PDT 2020

dschuff added inline comments.

================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:216
+            [llvm_i32_ty, llvm_i32_ty, llvm_ptr_ty, llvm_anyint_ty,
+             llvm_anyint_ty],
             [IntrWriteMem, IntrInaccessibleMemOrArgMemOnly, WriteOnly<ArgIndex<2>>,
----------------
tlively wrote:
> 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.
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82821