[PATCH] D120257: [AArch64][SME] Replace +streaming-sve feature with +streaming-compatible-<feature>.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64.td:441-460
+def FeatureStreamingCompatibleNEON
+    : SubtargetFeature<"streaming-compatible-neon",
+                       "HasStreamingCompatibleNEON", "true",
+                       "Enable subset of NEON instructions that are valid in "
+                       "both streaming SVE mode as well as regular mode",
+                       [FeatureFPARMv8, FeatureFullFP16]>;
+
----------------
sdesmalen wrote:
> c-rhodes wrote:
> > paulwalker-arm wrote:
> > > Do we need to split the feature flags based on other feature flag boundaries?  Why not just `FeatureStreamingCompatible`?  I'm not saying the above is wrong, but it does seem like an extra level of indirection so just trying to understand the reasoning.
> > > Do we need to split the feature flags based on other feature flag boundaries?  Why not just `FeatureStreamingCompatible`?  I'm not saying the above is wrong, but it does seem like an extra level of indirection so just trying to understand the reasoning.
> > 
> > I agree, naming aside I don't understand what the limitations of the existing feature flag are and why this extra granularity at the flag level is required.
> > 
> > 
> I don't think there is a reason that a single feature wouldn't work. My aim was mostly to remove the 'Streaming SVE' and 'SVE' from the name because 'Streaming SVE' has a special meaning in SME and the term 'SVE' in the name may incorrectly give the impression this is about SVE(1). And hence I started breaking this down into separate features to avoid the `SVE` suffix. But removing any suffix works as well  I guess.
> 
> I do expect that we'll have more instructions being added to the set of 'Streaming compatible' instructions over time. I wasn't sure if in C++ code it's easier to reason about "has featureABC or strict subset of featureABC" than it would to reason about "has featureABC or featureXYZ". Anyway, I'm happy to change it if you think that's an improvement. I guess it can always be split up in the future when there's a need for it.
Thanks @sdesmalen.  I would expect future additions to form part of a new spec so it would be treated much like any other extension in that we'd have `FeatureStreamingCompatibleVx.y` to match a specific level of the architecture/extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120257