This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Improve TargetParser API
ClosedPublic

Authored by tmatheson on Nov 28 2022, 4:51 AM.

Details

Summary

The TargetParser depends heavily on a collection of macros and enums to tie
together information about architectures, CPUs and extensions. Over time this
has led to some pretty awkward API choices. For example, recently a custom
operator-- has been added to the enum, which effectively turns iteration into
a graph traversal and makes the ordering of the macro calls in the header
significant. More generally there is a lot of string <-> enum conversion
going on. I think this shows the extent to which the current data structures
are constraining us, and the need for a rethink.

I tried to split this into smaller commits, but couldn't find a way to do it
without requiring people to review a lot of code that would be deleted in the
next patch.

AArch64 only for now, but if it is accepted I will work on the same for ARM.

Key changes:

  • Get rid of Arch enum, which is used to bind fields together. Instead of passing around ArchKind, use the named ArchInfo objects directly or via references.
  • The list of all known ArchInfo becomes an array of pointers.
  • ArchKind::operator-- is replaced with ArchInfo::implies(), which defines which architectures are predecessors to each other. This allows features from predecessor architectures to be added in a more intuitive way.
  • Free functions of the form f(ArchKind) are converted to ArchInfo::f(). Some functions become unnecessary and are deleted.
  • Version number and profile are added to the ArchInfo. This makes comparison of architectures easier and moves a couple of functions out of clang and into AArch64TargetParser.
  • clang::AArch64TargetInfo ArchInfo is initialised to Armv8a not INVALID.
  • AArch64::ArchProfile which is distinct from ARM::ArchProfile
  • Give things sensible names and add some comments.

Diff Detail

Event Timeline

tmatheson created this revision.Nov 28 2022, 4:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 4:51 AM
tmatheson requested review of this revision.Nov 28 2022, 4:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 28 2022, 4:51 AM
tmatheson edited the summary of this revision. (Show Details)Nov 28 2022, 4:53 AM
DavidSpickett accepted this revision.Nov 28 2022, 7:03 AM

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 AI for ArchInfo. I see that ArchKind was also a type and a var name. If this new name is following the codestyle then fine but I'd go with ArchInfo (arch_info but that's against the style, sigh). Up to you, I know the acronym style is common elsewhere and IDE helpers mitigate it somewhat.

llvm/include/llvm/Support/AArch64TargetParser.h
119

Do you need to do anything to prevent moving from this?

This revision is now accepted and ready to land.Nov 28 2022, 7:03 AM
tmatheson updated this revision to Diff 478617.Nov 29 2022, 8:42 AM

Rename AI -> ArchInfo and delete move constructor/assignment

tmatheson marked an inline comment as done.Nov 29 2022, 8:43 AM

Thanks for the review!

This revision was landed with ongoing or failed builds.Dec 1 2022, 4:51 AM
This revision was automatically updated to reflect the committed changes.

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?

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?

Hi, yes this change introduces a bunch of inline constexpr global variables in AArch64TargetParser.h.

bkramer added a subscriber: bkramer.Dec 4 2022, 3:17 AM
bkramer added inline comments.
llvm/include/llvm/Support/AArch64TargetParser.h
112–115

If this is really non-copyable add a (constexpr) constructor. non-copyable and aggregate initialization doesn't mix. C++20 doesn't allow it, C++17 accidentally has a lot of loopholes around the non-copyability if you do aggregate initialization.

155–162

Is there a good reason for these to be defined in the header? This was wrong before and now works because of inline constexpr, but it's still wasting a bunch of compile time.

Hahnfeld added inline comments.Dec 4 2022, 3:20 AM
llvm/include/llvm/Support/AArch64TargetParser.h
155–162

It's also likely that this is the reason for the failures I see with LLVM_LINK_LLVM_DYLIB, though I need to investigate more thoroughly what is going wrong in there...

mgorny added a subscriber: mgorny.Dec 4 2022, 9:07 AM

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?

Did you hit these?

Failed Tests (4):
  Clang :: Driver/aarch64-target-as-march.s
  Clang :: Misc/target-invalid-cpu-note.c
  Clang :: Preprocessor/aarch64-target-features.c
  Clang :: Sema/attr-target.c

I've just bisected them to this commit, so I guess yes.

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?

Did you hit these?

Failed Tests (4):
  Clang :: Driver/aarch64-target-as-march.s
  Clang :: Misc/target-invalid-cpu-note.c
  Clang :: Preprocessor/aarch64-target-features.c
  Clang :: Sema/attr-target.c

I've just bisected them to this commit, so I guess yes.

Yes, these are the ones I see in my "default" configuration (RelWithDebInfo and assertions turned on). If I however modify my Release configuration to add LLVM_LINK_LLVM_DYLIB, I see the following 9 failures:

Failed Tests (9):
  Clang :: Driver/aarch64-march.c
  Clang :: Driver/aarch64-sve.c
  Clang :: Driver/aarch64-sve2.c
  Clang :: Driver/aarch64-target-as-march.s
  Clang :: Driver/aarch64-v91a.c
  Clang :: Driver/aarch64-v92a.c
  Clang :: Misc/target-invalid-cpu-note.c
  Clang :: Preprocessor/aarch64-target-features.c
  Clang :: Sema/attr-target.c

It's not entirely clear to me why there are more failing tests...

@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.

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.

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.

tmatheson added inline comments.Dec 4 2022, 1:08 PM
llvm/include/llvm/Support/AArch64TargetParser.h
155–162

Is there a good reason for these to be defined in the header? This was wrong before and now works because of inline constexpr, but it's still wasting a bunch of compile time.

I'm not aware of a good reason. Doing something to improve the compile time impact is on the list of things I'd like to get to. They need to be declared in the header because they are used for comparisons (open to other suggestions) but don't have to be defined there.

I expected inline constexpr to guarantee the same address across shared libraries, but it looks like maybe it doesn't?

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:
llvm-project/llvm/include/llvm/Support/AArch64TargetParser.def:20:47: error: constexpr variable cannot have non-literal type 'const ArchInfo'
AARCH64_ARCH(0, 0, InvalidProfile, "invalid", INVALID, "+",

I intend to revert this change early 5 December. Please let me know if that will be an issue.

@bkramer pushed a C++20 fix which was reverted by 8be0d8fb83aa359caf0a7413c4a0fe49aef0dff1 by @tmatheson , but the revert had no message about why.

@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.

@saugustine I have reverted the patch while I address the issues that you and others have raised. In future if would be helpful if you provided some information about how to reproduce the error. However it looks like this is also related to C++20, and I would like to understand what the policy is there. As far as I am aware this is an untested configuration. If you are building with C++20 and any otherwise-good patches that break your build must be reverted, then there should be a buildbot covering that configuration. Without a buildbot, I think the onus should be on you to suggesting a fix, or at least give enough details to reproduce the problem and allow time for a fix. Apologies if I have misunderstood something.

lenary reopened this revision.Dec 16 2022, 6:44 AM
This revision is now accepted and ready to land.Dec 16 2022, 6:44 AM

@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.

That happened! Don't worry.

@saugustine I have reverted the patch while I address the issues that you and others have raised. In future if would be helpful if you provided some information about how to reproduce the error. However it looks like this is also related to C++20, and I would like to understand what the policy is there. As far as I am aware this is an untested configuration. If you are building with C++20 and any otherwise-good patches that break your build must be reverted, then there should be a buildbot covering that configuration. Without a buildbot, I think the onus should be on you to suggesting a fix, or at least give enough details to reproduce the problem and allow time for a fix. Apologies if I have misunderstood something.

Thanks! You can set -DCMAKE_CXX_STANDARD=20 to test C++20. https://raw.githubusercontent.com/chromium/chromium/main/tools/clang/scripts/update.py has a prebuilt Clang which can be used as CMAKE_{C,CXX}_COMPILER.
A build bot is in plan https://discourse.llvm.org/t/adding-a-c-20-buildbot/67156

Matt added a subscriber: Matt.Dec 19 2022, 8:46 AM
tmatheson updated this revision to Diff 488770.Jan 12 2023, 2:11 PM

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

Inline calls for the following macros and delete AArch64TargetParser.def:

AARCH64_ARCH,  AARCH64_CPU_NAME,  AARCH64_CPU_ALIAS, AARCH64_ARCH_EXT_NAME

Don't use address comparison, use string comparison instead.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 2:11 PM
tmatheson updated this revision to Diff 488945.Jan 13 2023, 4:34 AM

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

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"

The ArchInfo class is no longer non-copyable to satisfy C++20. The alternative of adding a constructor would have resulted in global constructor calls, which is not allowed in Support (the same presumably applies to the new TargetParser library).

I've tested locally with LLVM_BUILD_LLVM_DYLIB=ON + LLVM_LINK_LLVM_DYLIB=ON + CMAKE_CXX_STANDARD=20.

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

pratlucas accepted this revision.Jan 13 2023, 7:02 AM

LGTM with a tiny nit. Feel free to fix it when landing the changes.

llvm/include/llvm/TargetParser/AArch64TargetParser.h
164–166 ↗(On Diff #488945)

Minor nit: can you replace the // ????? comments with a single TODO to document those better? It'd look a bit less confusing for others reading this in the future.

danielkiss added inline comments.Jan 13 2023, 8:04 AM
clang/lib/Basic/Targets/AArch64.cpp
445
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
167

Would be nice to add a custom operator to ArchInfo to say *ArchInfo >= llvm::AArch64::ARMV9A
because it looks to me here the llvm::AArch64::ARMV9_3A and llvm::AArch64::ARMV9_4A are missing.

tmatheson added inline comments.Jan 13 2023, 8:35 AM
clang/lib/Basic/Targets/AArch64.cpp
445

I'll fix this when I push

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
167

Good catch. This could be written as if(ArchInfo.implies(ARMV9A)), but I'll leave that for a follow up patch. I opted against a custom operator because they generally make things less understandable, except in cases where the ordering is very obvious, e.g. numeric types. For example 9.2 does not imply 8.8. If you want to do an actual numerical comparison of version numbers you can compare ArchInfo.Versions directly.

tmatheson updated this revision to Diff 489221.Jan 14 2023, 3:47 AM

Address comments and fix flang test

Herald added a project: Restricted Project. · View Herald Transcript
tmatheson marked 2 inline comments as done.Jan 14 2023, 3:49 AM
This revision was landed with ongoing or failed builds.Jan 14 2023, 6:44 AM
This revision was automatically updated to reflect the committed changes.