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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to