[Lldb-commits] [PATCH] D138792: [AArch64] Improve TargetParser API

[Tomas Matheson via Phabricator via lldb-commits]

tmatheson added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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

[Lldb-commits] [PATCH] D138792: [AArch64] Improve TargetParser API

[Tomas Matheson via Phabricator via lldb-commits]

tmatheson added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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

[Lldb-commits] [PATCH] D138792: [AArch64] Improve TargetParser API

[Lucas Prates via Phabricator via lldb-commits]

pratlucas accepted this revision.
pratlucas added a comment.

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




Comment at: llvm/include/llvm/TargetParser/AArch64TargetParser.h:164-166
+  CPUFeatures CPUFeature;  // ?
+  StringRef DependentFeatures; // ?
+  unsigned FmvPriority;// ?

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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

[Lldb-commits] [PATCH] D138792: [AArch64] Improve TargetParser API

[Daniel Kiss via Phabricator via lldb-commits]

danielkiss added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:532
 getTargetDefinesARMV81A(Opts, Builder);
-break;
-  case llvm::AArch64::ArchKind::ARMV8_2A:
+  if (*ArchInfo == llvm::AArch64::ARMV8_2A)
 getTargetDefinesARMV82A(Opts, Builder);





Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:172
+   *ArchInfo == llvm::AArch64::ARMV9_1A ||
+   *ArchInfo == llvm::AArch64::ARMV9_2A)) {
 Features.push_back("+sve");

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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

[Lldb-commits] [PATCH] D138792: [AArch64] Improve TargetParser API

[Tomas Matheson via Phabricator via lldb-commits]

tmatheson added inline comments.



Comment at: clang/lib/Basic/Targets/AArch64.cpp:532
 getTargetDefinesARMV81A(Opts, Builder);
-break;
-  case llvm::AArch64::ArchKind::ARMV8_2A:
+  if (*ArchInfo == llvm::AArch64::ARMV8_2A)
 getTargetDefinesARMV82A(Opts, Builder);

danielkiss wrote:
> 
I'll fix this when I push



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:172
+   *ArchInfo == llvm::AArch64::ARMV9_1A ||
+   *ArchInfo == llvm::AArch64::ARMV9_2A)) {
 Features.push_back("+sve");

danielkiss wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138792

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