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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
