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