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

[Heejin Ahn via Phabricator via cfe-commits]

aheejin accepted this revision.
aheejin added inline comments.
This revision is now accepted and ready to land.


================
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());
----------------
tlively wrote:
> 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.
> `llvm_unreachable` is appropriate here because non-constant expressions will have been caught earlier by the type checking.
Oh, does `Ii` ensures that? That's cool...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57736