Page MenuHomePhabricator

[clang][NFC] Fix a potential assert failure
ClosedPublic

Authored by benshi001 on Apr 15 2021, 8:14 PM.

Details

Summary

The calculation of LargestBuiltinID needs all targets information.

Diff Detail

Event Timeline

benshi001 created this revision.Apr 15 2021, 8:14 PM
benshi001 requested review of this revision.Apr 15 2021, 8:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 8:14 PM
MaskRay accepted this revision.Apr 15 2021, 8:49 PM

SVE::FirstTSBuiltin is 8148, the largest.

This revision is now accepted and ready to land.Apr 15 2021, 8:49 PM

SVE::FirstTSBuiltin is 8148, the largest.

Isn't SVE::FirstTSBuiltin used to start AArch64's builtins list. So shouldn't AArch64::LastTSBuiltin be larger?

SVE::FirstTSBuiltin is 8148, the largest.

Isn't SVE::FirstTSBuiltin used to start AArch64's builtins list. So shouldn't AArch64::LastTSBuiltin be larger?

You are right. I misspoke. AArch64::LastTSBuiltin is 8328.

SVE::FirstTSBuiltin is 8148, the largest.

Isn't SVE::FirstTSBuiltin used to start AArch64's builtins list. So shouldn't AArch64::LastTSBuiltin be larger?

You are right. I misspoke. AArch64::LastTSBuiltin is 8328.

That raises the question of why the NEON and SVE are in this list std::max at all. They're only helpers for generating the correct information for ARM and AArch64 which will always be larger.

MaskRay added inline comments.Apr 15 2021, 9:31 PM
clang/include/clang/Basic/TargetBuiltins.h
333–334

SVE::FirstTSBuiltin can be dropped.

benshi001 updated this revision to Diff 338045.Apr 16 2021, 3:06 AM
benshi001 retitled this revision from [clang] Fix a potential assert failure to [clang][NFC] Fix a potential assert failure.
benshi001 added a reviewer: craig.topper.
benshi001 marked an inline comment as done.
benshi001 added inline comments.
clang/include/clang/Basic/TargetBuiltins.h
333–334

SVE and NEON are removed, VE and RISCV are added.

Can I commit it?

craig.topper accepted this revision.Apr 16 2021, 2:32 PM

LGTM. Thanks for removing the NEON and SVE.

This revision was automatically updated to reflect the committed changes.
benshi001 marked an inline comment as done.