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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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]>;
+
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp:60-69
-  // Most of the NEON instruction set isn't supported in streaming mode on SME
-  // targets, disable NEON unless explicitly requested.
-  bool RequestedNEON = FS.contains("neon");
-  bool RequestedStreamingSVE = FS.contains("streaming-sve");
-  MCSubtargetInfo *STI =
-      createAArch64MCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS);
-  if (RequestedStreamingSVE && !RequestedNEON &&
----------------
c-rhodes wrote:
> why is this removed?
Given that the streaming mode feature is something internal to the compiler (and not a feature flag that can be set through -march, because only `+sme`, `+sme-i64` and `+sme-f64` are user facing), we can have front-end toggle these features rather than doing it implicitly here when creating the subtarget. Through the ACLE it will be possible to define a function as 'streaming compatible' using an attribute. Clang could then set the appropriate `-neon,+streaming-compatible-sve` feature flags for the IR function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120257