[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

Mon Jul 29 12:38:05 PDT 2019

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