[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added inline comments. Comment at: clang/lib/Basic/Targets/AArch64.cpp:532 getTargetDefinesARMV81A(Opts, Builder); -break; - case llvm::AArch64::ArchKind::ARMV8_2A: + if (*ArchInfo == llvm::AArch64::ARMV8_2A) getTargetDefinesARMV82A(Opts, Builder);

[PATCH] D138792: [AArch64] Improve TargetParser API

danielkiss added inline comments. Comment at: clang/lib/Basic/Targets/AArch64.cpp:532 getTargetDefinesARMV81A(Opts, Builder); -break; - case llvm::AArch64::ArchKind::ARMV8_2A: + if (*ArchInfo == llvm::AArch64::ARMV8_2A) getTargetDefinesARMV82A(Opts, Builder);

[PATCH] D138792: [AArch64] Improve TargetParser API

pratlucas accepted this revision. pratlucas added a comment. LGTM with a tiny nit. Feel free to fix it when landing the changes. Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:164-166 + CPUFeatures CPUFeature; // ? + StringRef DependentFeatures; //

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. Worth noting that this had to be reworked because both D127812 and D137838 went in since this was reverted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. The most recent versions of this patch contains squashed changes from these reviews: - D139278 "[AArch64] Use string comparison for ArchInfo equality." This fixes the test failures with shared libraries, which were caused by each

[PATCH] D138792: [AArch64] Improve TargetParser API

MaskRay added a comment. In D138792#3970376 , @tmatheson wrote: > @MaskRay I reverted that commit because it broke important functionality > (comparison by address) to fix an issue in an unsupported C++ version, it > wasn't reviewed, and it was not

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. @MaskRay I reverted that commit because it broke important functionality (comparison by address) to fix an issue in an unsupported C++ version, it wasn't reviewed, and it was not clear from the commit message what it was fixing. I explained this in a comment on the

[PATCH] D138792: [AArch64] Improve TargetParser API

MaskRay added a comment. @bkramer pushed a C++20 fix which was reverted by 8be0d8fb83aa359caf0a7413c4a0fe49aef0dff1 by @tmatheson , but the revert had no message about why. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D138792: [AArch64] Improve TargetParser API

saugustine added a comment. We are seeing constexpr failures with this change. They look like this: In file included from llvm-project/llvm/lib/Support/AArch64TargetParser.cpp:14: In file included from llvm-project/llvm/include/llvm/Support/AArch64TargetParser.h:160:

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:154-160 +// Create ArchInfo structs named +#define AARCH64_ARCH(MAJOR, MINOR, PROFILE, NAME, ID, ARCH_FEATURE, \ + ARCH_BASE_EXT)

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. Yes I will look into it and address the other comments when I have more time tomorrow or later this week. However I'm starting to think that the comparison by address is too easy to subtly break, and not immediately obvious to debug, and is therefore not worth it in

[PATCH] D138792: [AArch64] Improve TargetParser API

Hahnfeld added a comment. What about @bkramer's suggestion to move the definitions out of the header? Based on what you write (comparison by address), this should also solve the issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138792/new/

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. @Hahnfeld, @mgorny I was able to reproduce the failures with LLVM_LINK_LLVM_DYLIB, and they are failing because the comparison is failing because copies are being created. I don't fully understand how but presumably we are still ending up with one object per shared

[PATCH] D138792: [AArch64] Improve TargetParser API

Hahnfeld added a comment. In D138792#3969310 , @mgorny wrote: > In D138792#3966920 , @Hahnfeld > wrote: > >> Hi, I bisected this change to lead to a couple of test failures when >> building with

[PATCH] D138792: [AArch64] Improve TargetParser API

mgorny added a comment. In D138792#3966920 , @Hahnfeld wrote: > Hi, I bisected this change to lead to a couple of test failures when building > with `LLVM_LINK_LLVM_DYLIB`. In the past, this had to do with global variable > initialization order, but

[PATCH] D138792: [AArch64] Improve TargetParser API

Hahnfeld added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:154-160 +// Create ArchInfo structs named +#define AARCH64_ARCH(MAJOR, MINOR, PROFILE, NAME, ID, ARCH_FEATURE, \ + ARCH_BASE_EXT)

[PATCH] D138792: [AArch64] Improve TargetParser API

bkramer added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:115-118 + ArchInfo(const ArchInfo &) = delete; + ArchInfo(const ArchInfo &&) = delete; + ArchInfo =(const ArchInfo ) = delete; + ArchInfo &=(const ArchInfo &) = delete;

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson added a comment. In D138792#3966920 , @Hahnfeld wrote: > Hi, I bisected this change to lead to a couple of test failures when building > with `LLVM_LINK_LLVM_DYLIB`. In the past, this had to do with global variable > initialization order, but

[PATCH] D138792: [AArch64] Improve TargetParser API

Hahnfeld added a comment. Hi, I bisected this change to lead to a couple of test failures when building with `LLVM_LINK_LLVM_DYLIB`. In the past, this had to do with global variable initialization order, but nothing immediately jumps to my eye in this patch. Is `AARCH64_ARCH` used to define

[PATCH] D138792: [AArch64] Improve TargetParser API

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG450de8008bb0: [AArch64] Improve TargetParser API (authored by tmatheson). Changed prior to commit:

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson marked an inline comment as done. tmatheson added a comment. Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138792/new/ https://reviews.llvm.org/D138792 ___ cfe-commits

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson updated this revision to Diff 478617. tmatheson added a comment. Rename AI -> ArchInfo and delete move constructor/assignment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138792/new/ https://reviews.llvm.org/D138792 Files:

[PATCH] D138792: [AArch64] Improve TargetParser API

DavidSpickett accepted this revision. DavidSpickett added a comment. This revision is now accepted and ready to land. Looks like a good direction to me. Plenty more bits in clang that should really live in the target parser, and this is a great start at moving those. My only issue is the use of

[PATCH] D138792: [AArch64] Improve TargetParser API

tmatheson created this revision. tmatheson added reviewers: lenary, pratlucas, dmgreen, tschuett, DavidSpickett, danielkiss. Herald added subscribers: hiraditya, kristof.beyls. Herald added a project: All. tmatheson requested review of this revision. Herald added subscribers: llvm-commits,