This is an archive of the discontinued LLVM Phabricator instance.

[COFF, ARM64] Reorder handling of aarch64 MSVC builtins
ClosedPublic

Authored by dmajor on Jul 29 2019, 9:46 AM.

Details

Summary

In CodeGenFunction::EmitAArch64BuiltinExpr(), bulk move of all of the aarch64 MSVC-builtin cases to an earlier point in the function (the // Handle non-overloaded intrinsics first. switch block) in order to avoid an unreachable in GetNeonType(). The NEON type-overloading logic is not appropriate for the Windows builtins.

Fixes https://llvm.org/pr42775

Diff Detail

Repository
rL LLVM

Event Timeline

dmajor created this revision.Jul 29 2019, 9:46 AM
efriedma added inline comments.Jul 29 2019, 12:09 PM
lib/CodeGen/CGBuiltin.cpp
8182 ↗(On Diff #212181)

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"?

dmajor marked an inline comment as done.Jul 29 2019, 12:26 PM
dmajor added inline comments.
lib/CodeGen/CGBuiltin.cpp
8182 ↗(On Diff #212181)

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.

efriedma accepted this revision.Jul 29 2019, 12:34 PM

LGTM

lib/CodeGen/CGBuiltin.cpp
8182 ↗(On Diff #212181)

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.

This revision is now accepted and ready to land.Jul 29 2019, 12:34 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 8:35 AM