This is an archive of the discontinued LLVM Phabricator instance.

Differential D100616

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

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

Details

Reviewers
MaskRay
craig.topper
Commits
rG06995fe256ec: [clang][NFC] Fix a potential assert failure
Summary

The calculation of LargestBuiltinID needs all targets information.

Diff Detail

Event Timeline

benshi001 created this revision.Apr 15 2021, 8:14 PM
Herald added subscribers: s.egerton, simoncook, dschuff. · View Herald TranscriptApr 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
Herald added subscribers: cfe-commits, aheejin. · View Herald Transcript
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
Harbormaster completed remote builds in B99070: Diff 337971.Apr 15 2021, 8:51 PM
craig.topper added a subscriber: craig.topper.Apr 15 2021, 8:59 PM
In D100616#2693595, @MaskRay wrote:

SVE::FirstTSBuiltin is 8148, the largest.

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

MaskRay added a comment.Apr 15 2021, 9:05 PM
In D100616#2693603, @craig.topper wrote:
In D100616#2693595, @MaskRay wrote:

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.

craig.topper added a comment.Apr 15 2021, 9:09 PM
In D100616#2693607, @MaskRay wrote:
In D100616#2693603, @craig.topper wrote:
In D100616#2693595, @MaskRay wrote:

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?

Harbormaster completed remote builds in B99125: Diff 338045.Apr 16 2021, 4:01 AM
craig.topper accepted this revision.Apr 16 2021, 2:32 PM

LGTM. Thanks for removing the NEON and SVE.

Closed by commit rG06995fe256ec: [clang][NFC] Fix a potential assert failure (authored by benshi001). · Explain WhyApr 16 2021, 4:17 PM
This revision was automatically updated to reflect the committed changes.
benshi001 marked an inline comment as done.
benshi001 added a commit: rG06995fe256ec: [clang][NFC] Fix a potential assert failure.

Revision Contents

PathSize
clang/
include/
clang/
Basic/
6 lines

Diff 338258

clang/include/clang/Basic/TargetBuiltins.h

Show First 20 Lines • Show All 324 Lines • ▼ Show 20 Lines namespace WebAssembly {
enum { enum {
LastTIBuiltin = clang::Builtin::FirstTSBuiltin-1, LastTIBuiltin = clang::Builtin::FirstTSBuiltin-1,
#define BUILTIN(ID, TYPE, ATTRS) BI##ID, #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
#include "clang/Basic/BuiltinsWebAssembly.def" #include "clang/Basic/BuiltinsWebAssembly.def"
LastTSBuiltin LastTSBuiltin
}; };
} }
static constexpr uint64_t LargestBuiltinID = std::max<uint64_t>( static constexpr uint64_t LargestBuiltinID = std::max<uint64_t>(
{NEON::FirstTSBuiltin, ARM::LastTSBuiltin, SVE::FirstTSBuiltin, {ARM::LastTSBuiltin, AArch64::LastTSBuiltin, BPF::LastTSBuiltin,
MaskRayUnsubmitted
Done
ReplyInline Actions

SVE::FirstTSBuiltin can be dropped.

MaskRay: SVE::FirstTSBuiltin can be dropped.
benshi001AuthorUnsubmitted
Done
ReplyInline Actions

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

Can I commit it?

benshi001: SVE and NEON are removed, VE and RISCV are added. Can I commit it?
AArch64::LastTSBuiltin, BPF::LastTSBuiltin, PPC::LastTSBuiltin, PPC::LastTSBuiltin, NVPTX::LastTSBuiltin, AMDGPU::LastTSBuiltin,
NVPTX::LastTSBuiltin, AMDGPU::LastTSBuiltin, X86::LastTSBuiltin, X86::LastTSBuiltin, VE::LastTSBuiltin, RISCV::LastTSBuiltin,
Hexagon::LastTSBuiltin, Mips::LastTSBuiltin, XCore::LastTSBuiltin, Hexagon::LastTSBuiltin, Mips::LastTSBuiltin, XCore::LastTSBuiltin,
Le64::LastTSBuiltin, SystemZ::LastTSBuiltin, Le64::LastTSBuiltin, SystemZ::LastTSBuiltin,
WebAssembly::LastTSBuiltin}); WebAssembly::LastTSBuiltin});
} // end namespace clang. } // end namespace clang.
#endif #endif