User Details
- User Since
- Dec 10 2020, 4:17 AM (120 w, 2 d)
Thu, Mar 30
Mon, Mar 27
LGTM, but I'm not that familiar with the code that selects the alignment so it would be good to get a second opinion.
Fri, Mar 24
Thu, Mar 23
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
Tue, Mar 21
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.
Fri, Mar 17
Mon, Mar 13
LGTM
Wed, Mar 8
LGTM, thanks for making these changes.
Tue, Mar 7
Mon, Mar 6
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
Fri, Mar 3
@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?
Feb 14 2023
Feb 13 2023
Feb 8 2023
Feb 7 2023
Use LSE2 for unordered/monotonic
Feb 1 2023
Jan 31 2023
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.
Jan 30 2023
Jan 27 2023
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.
Jan 26 2023
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.
Jan 25 2023
Jan 24 2023
Require LSE2
Jan 23 2023
Add comments clarifying asserts
Jan 20 2023
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?
Jan 19 2023
LGTM
Jan 14 2023
Landed as part of f4225d325c19ae0e5dbe39faa900d81e24559da0
Landed as part of f4225d325c19ae0e5dbe39faa900d81e24559da0
Address comments and fix flang test
Jan 13 2023
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
Jan 12 2023
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.
Jan 10 2023
Jan 5 2023
Dec 23 2022
I am still seeing this linking error when building with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS: /unittests/Analysis/InlineAdvisorPlugin.so: undefined symbol: _ZN4llvm13AllAnalysesOnINS_6ModuleEE6SetKeyEStack.
Dec 20 2022
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({});
Dec 16 2022
Dec 5 2022
@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.
Dec 4 2022
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,
Dec 2 2022
And this is the file that was being generated: https://github.com/ziglang/zig/blob/c86589a738e5053a26b2be7d156bcfee1565f00b/lib/std/target/aarch64.zig
Update lldb test