[PATCH] D131058: [AArch64] Add an error if SVE scalable vector types are used in a context without sve

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:676
+    const std::vector<std::string> &FeaturesVec) const {
+  // FIXME: This should be based on the existing feature map in the backend.
+  std::vector<std::string> UpdatedFeaturesVec;
----------------
sdesmalen wrote:
> Is it possible to address the FIXME as part of this patch? I'm a bit worried about more technical debt creeping in here, as handling the feature flags is already quite complex and replicated in different places. For example, lib/Driver/ToolChains/Arch/AArch64.cpp has a function `DecodeAArch64Features` which attempts to do something similar.
Ah I see. That is the kind of thing I was hoping to avoid. The backend already knows the map of target features that depend on other features, but it's not available to TargetParser at the moment. We might need to add it in the end, but the entire feature map is really a large problem that cannot be solved very quickly. DecodeAArch64Features looks like quite a small subset of the full dependencies that we'll probably need for feature mapping. See the X86 examples in the Implied features in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/X86TargetParser.cpp.

Part of this work will hopefully involve fixing how target(..) attributes work. They currently assume a X86 model, where `arch=cpu` and there is no `cpu=..`. There is no way (I don't think) to base a target feature off an architecture version, which will be needed for some of the arm_neon.h intrinsics.

So a lot of this will need to change. If you consider the target features not always coming from -march options but alternatively from target(..) attributes then the right place for this is moved away from Driver and into this TargetInfo class. I have changed this slightly, to work with setFeatureEnabled as opposed to initFeatureMap. I was hoping that this would allow the code in DecodeAArch64Features to be removed, but the tests seem to be asserting that +sve2+nosve2 should enable sve, which this would apparently not handle.


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

https://reviews.llvm.org/D131058