[PATCH] D138792: [AArch64] Improve TargetParser API

[Tomas Matheson via Phabricator via cfe-commits]

tmatheson added inline comments.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.h:154-160
+// Create ArchInfo structs named <ID>
+#define AARCH64_ARCH(MAJOR, MINOR, PROFILE, NAME, ID, ARCH_FEATURE,            \
+                     ARCH_BASE_EXT)                                            \
+  inline constexpr ArchInfo ID = {VersionTuple{MAJOR, MINOR}, PROFILE, NAME,   \
+                                  ARCH_FEATURE, ARCH_BASE_EXT};
+#include "AArch64TargetParser.def"
+#undef AARCH64_ARCH
----------------
Hahnfeld wrote:
> bkramer wrote:
> > 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.
> 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...
> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138792/new/

https://reviews.llvm.org/D138792