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