LGTM, but I'm not that familiar with the code that selects the alignment so it would be good to get a second opinion.

This test fails when I build and run with the following:

cmake ../llvm -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON -DLLVM_ENABLE_PLUGINS=ON -DBUILD_SHARED_LIBS=OFF -DLLVM_ENABLE_PROJECTS=clang
ninja unittests/Analysis/AnalysisTests
./unittests/Analysis/AnalysisTests --gtest_filter=PluginInlineOrderTest.NoInlineFoo

I get the following failure:

$ ./unittests/Analysis/AnalysisTests --gtest_filter=PluginInlineOrderTest.NoInlineFoo
Note: Google Test filter = PluginInlineOrderTest.NoInlineFoo
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from PluginInlineOrderTest
[ RUN      ] PluginInlineOrderTest.NoInlineFoo
/ssd/upstream/llvm/unittests/Analysis/PluginInlineOrderAnalysisTest.cpp:48: Failure
Value of: !!Plugin
  Actual: false
Expected: true
Plugin path: /ssd/upstream/build2/unittests/Analysis/InlineOrderPlugin.so
[  FAILED  ] PluginInlineOrderTest.NoInlineFoo (4 ms)
[----------] 1 test from PluginInlineOrderTest (4 ms total)

LGTM

Looks sensible but I don't fully understand the context of the change. Please could you explain more what is wrong with the current behaviour, and which parts of the AAPCS you are referring to.

LGTM

LGTM, thanks for making these changes.

For anyone reading, there's more context here about std::optional::value(): https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

@mehdi_amini can you explain what the original reasoning behind this restriction was for Support, and whether we need to make the same restriction for TargetParser now that it's split out?

I'm not sure that this is the right fix. The exception indicates that this is being called with ArchInfo objects with invalid VersionTuples, how is that happening? Also could you explain what the issue is with bad_optional_access on older macOS?

Use LSE2 for unordered/monotonic

In D142963#4094545, @andrewrk wrote:

Speaking as the one who filed the motivating bug report, all of the above behaviors are fine. The motivating use case is explicitly specifying a full set of enabled/disabled features, leaving nothing changed by LLVM's own dependency resolution. In this use case, LLVM would never see any of these three scenarios as input.

By the way I don't really have any more to add, and I've lost track of what the status of +crypto is after all the recent changes, so don't let me block this. From what you've said aligning with GCC sounds sensible, but an intermediate stage where -march does one thing and the target attribute does another seems less ideal.

It looks like this has these odd behaviours:

• -target-feature +v9.3a -target-feature -v9.2a will remove v9.2a but add all the dependent features of both v9.3a and v9.2a. It also doesn't remove v9.3a which implies v9.2a.
• -target-feature -v9.2a -target-feature +v9.3a will add the v9.2a back, and also add all the dependent features of both v9.3a and v9.2a.
• -target-feature +v9.3a -target-feature -v9.3a will remove v9.3a but add all the dependent features of v9.3a.

@efriedma D142712 Like this?

This patch has made it considerably harder to understand what is going on in the TargetParser. If you get a chance, please could you add some clarifying comments and tidy-ups. I appreciate that a lot of this is following the lead of the pre-existing TargetParser code, but lets try to improve it as we go.

Looks great, thanks

Yes this case is tested for implies(): EXPECT_TRUE(AArch64::ARMV9_4A.implies(AArch64::ARMV8_9A));

No, FEAT_LSE128 only implies FEAT_LSE, not FEAT_LSE2.

Require LSE2

D141406

Add comments clarifying asserts

I'm concerned that the +crypto situation is already confusing, and adding another different meaning for it in the context of the target(...) attribute exacerbates it. Is the plan for this to eventually match the behaviour of -march=...+crypto?

LGTM

Landed as part of f4225d325c19ae0e5dbe39faa900d81e24559da0

Landed as part of f4225d325c19ae0e5dbe39faa900d81e24559da0

Address comments and fix flang test

Worth noting that this had to be reworked because both D127812 and D137838 went in since this was reverted.

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 shared library ending up with it's own copy of the ArchInfo instances and hence breaking the equality-by-address.
• D139102 "[AArch64] Inline AArch64TargetParser.def"

Fix clang/test/Sema/attr-target-clones-aarch64.c

Reworked after several other major changes to the TargetParser since
this was reverted. Combined with several other approved changes.

It does, I need to check that the old tests are definitely replicated here before removing them.

I agree the approach in D141518 makes more sense

LGTM, makes sense to move into TP, D141404 should do the same it it makes sense.

I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.

Couple of nits, since you will be updating this anyway after dropping D140221, otherwise LGTM.

I'm not convinced this gives us much over using InnerT=optional directly; arguably it just obfuscates the type. There are no examples in the codebase for StringSwitch<std::optional>, but there are about 80 of StringSwitch<llvm::Optional>, and they typically look like this:

auto Level = llvm::StringSwitch<Optional<Driver::ReproLevel>>(A->getValue())
                 .Case("off", Driver::ReproLevel::Off)
                 .Case("crash", Driver::ReproLevel::OnCrash)
                 .Case("error", Driver::ReproLevel::OnError)
                 .Case("always", Driver::ReproLevel::Always)
                 .Default(None);

The std::optional equivalent can be written pretty much the same. In your before example, you don't need explicit std::optional constructor calls, you can use auto because they type is obvious from the RHS, and you can use the default constructor for std::optional (debatable whether that is preferable to std::nullopt):

auto Foo = llvm::StringSwitch<std::optional<InnerT>>(str)
    .Case("foo", InnerT(...))
    .Default({});

@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 original commit but forgot to add it to the message for the revert, sorry.

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 this case. The performance of the comparison is not especially critical here afaik.

@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 library. I tried adding a constructor as @bkramer suggested but this did not solve the issue. Please see D139278 for a fix.

I don't see the point of disallowing copying an aggregate,

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 nothing immediately jumps to my eye in this patch. Is AARCH64_ARCH used to define global variables?

And this is the file that was being generated: https://github.com/ziglang/zig/blob/c86589a738e5053a26b2be7d156bcfee1565f00b/lib/std/target/aarch64.zig

Update lldb test