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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
