[PATCH] D83079: [clang] Default target features implied by `-march` on AArch64.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli marked an inline comment as done.
fpetrogalli added a comment.

In D83079#2129371 <https://reviews.llvm.org/D83079#2129371>, @SjoerdMeijer wrote:

> I haven't looked into details of these arch extensions yet, will do that tomorrow, but I don't think there's any disagreement, i.e., these options and their behaviour are synchronised with the GCC community.  Thus, it's better if you remove this wording from both description and comments in the source code. If there is divergence in behaviour, then that is probably be an oversight somewhere. I am also pretty sure that this 8.5 stuff is implemented, not sure if that is upstreamed. And also, I think we need to be more specific than "Default target features implied by `-march` on AArch64", because this is a big topic, which is not addressed in this patch.  This patch is only about a few architecture extension. I am also for example surprised to see bfloat there, as I saw quite some activity in this area.


I @SjoerdMeijer  - I think I misinterpreted the way the target feature are generated, and which part of the codebase handles that. I wasn't aware about the common code pointed out by @sdesmalen that was translating architecture version into target feature - I thought everything needed to be translated by the driver into a list of `-target-feature`, that is why I thought there was discrepancy. If there is any, this patch is probably not the right way to fix it. The best thing to do is abandon this patch.



================
Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:118
+  case llvm::AArch64::ArchKind::ARMV8_6A:
+    Features.push_back("+i8mm");
+    Features.push_back("+bf16");
----------------
sdesmalen wrote:
> Looking at what Clang emits for e.g. `-march=armv8.5-a`, it just adds a target-feature `+v8.5a`. The definitions in `llvm/lib/Target/AArch64/AArch64.td`. suggests that LLVM is already able to infer all supported features from that. e.g.
> ```
> def HasV8_4aOps : SubtargetFeature<
>    :
>    :
> 
> def HasV8_5aOps : SubtargetFeature<
>   "v8.5a", "HasV8_5aOps", "true", "Support ARM v8.5a instructions",
>   [HasV8_4aOps, FeatureAltFPCmp, FeatureFRInt3264, FeatureSpecRestrict,
>    FeatureSSBS, FeatureSB, FeaturePredRes, FeatureCacheDeepPersist,
>    FeatureBranchTargetId]>;
> 
> def HasV8_6aOps : SubtargetFeature<
>   "v8.6a", "HasV8_6aOps", "true", "Support ARM v8.6a instructions",
> 
>   [HasV8_5aOps, FeatureAMVS, FeatureBF16, FeatureFineGrainedTraps,
>    FeatureEnhancedCounterVirtualization, FeatureMatMulInt8]>;
> ```
> So I don't think you necessarily have to decompose the architecture version into target-features in the Clang driver as well. For Clang it matters that the right set of feature macros are defined so that the ACLE header file exposes the correct set of functions for the given architecture version. At least for the SVE ACLE that is just a small handful of features.
Thank you for explaining this @sdesmalen, I thought that the features needed to get explicitly generated by clang.
I will create a separate patch that adds only the feature macros needed for the SVE ACLEs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83079