[PATCH] D57736: [WebAssembly] Bulk memory intrinsics and builtins

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.


================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:29
+// Bulk memory builtins
+TARGET_BUILTIN(__builtin_wasm_memory_init, "vIiv*ii", "", "bulk-memory")
+TARGET_BUILTIN(__builtin_wasm_data_drop, "vIi", "", "bulk-memory")
----------------
- When do we use `Ii` instead of `i`?
- Shouldn't we add the memory index field as well, even if that means a user always has to set it to 0? Are we gonna add a new builtin once multiple memories are available?
- Shouldn't the segment index, segment offset, and the size operands be `Ui` because they cannot be negative?


================
Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:30
+TARGET_BUILTIN(__builtin_wasm_memory_init, "vIiv*ii", "", "bulk-memory")
+TARGET_BUILTIN(__builtin_wasm_data_drop, "vIi", "", "bulk-memory")
+
----------------
The same thing..
- Shouldn't the segment index be `Ui`?
- Shouldn't we add the memory index field as well?



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13477
+    if (!SegArg->isIntegerConstantExpr(SegConst, getContext()))
+      llvm_unreachable("Constant arg isn't actually constant?");
+    llvm::Type *SegType = ConvertType(SegArg->getType());
----------------
Not sure if we can use `llvm_unreachable` here, because we can certainly reach here if a user uses this builtin with a non-const variable. In this file people often just used `assert` for user errors, which is not ideal either.

I haven't used it myself, but looking at the code, the recommended way to signal an error looks like to use [[ https://github.com/llvm/llvm-project/blob/db7fbcb038f095622a3e6847ecd6ff80bdc2483a/clang/lib/CodeGen/CodeGenFunction.h#L2092-L2094 | `CodeGenFunction::ErrorUnsupported` ]] function, as in [[ https://github.com/llvm/llvm-project/blob/0e04ebdcda44ef90e25abd0a2e65cc755ae8bc37/clang/lib/CodeGen/CGBuiltin.cpp#L2458-L2460 | here ]]. We used `llvm_unreachable` for SIMD builtins too, but maybe we can fix it later.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13488
+    Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_memory_init,
+                                        {SegType, IdxType, DstType});
+    return Builder.CreateCall(Callee, Args);
----------------
Do we need to pass types here to make intrinsic names overloaded like `llvm.wasm.memory.init.i32.i32.i8`, unless this intrinsic also support operands of other types, which it doesn't? The same for `data.drop` builtin.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:119
+  Intrinsic<[],
+            [llvm_anyint_ty, llvm_anyint_ty, LLVMPointerType<llvm_any_ty>,
+             llvm_i32_ty, llvm_i32_ty],
----------------
- Why are the first two immediates anyint? The spec says the segment index is varuint32 (so that will be `llvm_i32_ty` here), and for the memory index I don't think we are ever gonna need something larger than i32 either, but maybe it is better to be clarified in the spec too though.
- For the pointer type, I guess `llvm_ptr_ty` will be sufficient, unless we have multiple overloaded intrinsics of the same name.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:121
+             llvm_i32_ty, llvm_i32_ty],
+            [IntrWriteMem, IntrArgMemOnly, WriteOnly<1>],
+            "", [SDNPMemOperand]>;
----------------
- Isn't the pointer argument number not 1 but 2?
- I guess this should have `IntrHasSideEffects` as well, as in `data.drop`?
- I don't know much how they are handled differently in compilation, but because we can access some other memory, which holds the segment part, during execution, how about `IntrInaccessibleMemOrArgMemOnly` instead of `IntrArgMemOnly`?


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:122
+            [IntrWriteMem, IntrArgMemOnly, WriteOnly<1>],
+            "", [SDNPMemOperand]>;
+def int_wasm_data_drop :
----------------
I told you we needed this, but on second thought, because we don't really use `MachineMemOperand`, maybe we don't need it..? :$ The [[ https://github.com/llvm/llvm-project/blob/dc2c93017f8bf2a2c10b8e024f8f4a6409dbeeee/llvm/include/llvm/IR/Intrinsics.td#L483-L496 | standard memcpy/memmove/memset intrinsics ]] don't have it either. So if the code runs ok without these I think we can delete it. The same for `data.drop` intrinsic. Sorry for the incorrect info and confusion.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:125
+  Intrinsic<[],
+            [llvm_anyint_ty],
+            [IntrNoDuplicate, IntrHasSideEffects],
----------------
The same, `llvm_i32_ty`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57736