efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: lib/CodeGen/CGBuiltin.cpp:8182
switch (BuiltinID) {
default: return nullptr;
case NEON::BI__builtin_neon_vbsl_v:
----------------
dmajor wrote:
> efriedma wrote:
> > I'm a little concerned about the overall code structure here; even if
> > moving the code for the MSVC builtins is enough to fix those builtins
> > specifically, is it actually impossible to hit this "default"? If it is,
> > can we convert it to an "unreachable"?
> I'm not sure if this question was directed to me... this was a drive-by patch
> from my end so I'm not familiar with what other types of builtins there might
> be.
>
> I should probably mention that I'm hoping to get a fix merged to 9.0 in order
> to unblock Firefox. Unless someone can tell me that the unreachable is
> definitely safe, I'd worry about adding instability into the release branch.
> Perhaps the unreachable could be done in a separate commit only on 10.0 trunk
> where the tolerance for surprises is generally better.
>
It should be impossible to reach this normally, I think; any target-specific
builtin which codegen supports should be handled earlier, and target-specific
builtins only exist on the relevant target. The issue would just be a weird
crash if you add a new builtin Builtins.def without code generation support,
instead of a nicer "unsupported" error.
But I'm okay taking this as-is without trying to refactor the code to handle
that gracefully, if we need this for 9.0.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65403/new/
https://reviews.llvm.org/D65403
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
