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

[Thomas Lively via Phabricator via cfe-commits]

tlively added inline comments.


================
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")
----------------
aheejin wrote:
> - 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?
We use `Ii` instead of `i` when the argument needs to be a compile time constant because it is encoded as a literal in the corresponding instruction.

I was imagining that we could add a new builtin once multiple memories are available, but perhaps it would be better to add an argument to the builtin now and error out if it is not zero. I will do that.

Yes, `Ui` seems reasonable.




================
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")
+
----------------
aheejin wrote:
> The same thing..
> - Shouldn't the segment index be `Ui`?
> - Shouldn't we add the memory index field as well?
> 
 - Done

 - This intrinsic doesn't have a memory index, since it doesn't write anything into memory.


================
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());
----------------
aheejin wrote:
> 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.
`llvm_unreachable` is appropriate here because non-constant expressions will have been caught earlier by the type checking.

A follow-up PR updating SIMD intrinsics to use `Ui` and appropriate error handling sounds good.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13488
+    Function *Callee = CGM.getIntrinsic(Intrinsic::wasm_memory_init,
+                                        {SegType, IdxType, DstType});
+    return Builder.CreateCall(Callee, Args);
----------------
aheejin wrote:
> 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.
Fixed. I had been copying `llvm.memcpy`, but I agree it is better to not be polymorphic.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:121
+             llvm_i32_ty, llvm_i32_ty],
+            [IntrWriteMem, IntrArgMemOnly, WriteOnly<1>],
+            "", [SDNPMemOperand]>;
----------------
aheejin wrote:
> - 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`?
- Yes, good catch.

- Ok, I'm not sure it's necessary but it can't hurt.

- I'm not sure this is necessary either, but again it can't hurt. Better safe than sorry.


================
Comment at: llvm/include/llvm/IR/IntrinsicsWebAssembly.td:122
+            [IntrWriteMem, IntrArgMemOnly, WriteOnly<1>],
+            "", [SDNPMemOperand]>;
+def int_wasm_data_drop :
----------------
aheejin wrote:
> 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.
No problem!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57736