[PATCH] D121208: [AArch64][SME] Split up SME features. (alternative approach)

[Richard Sandiford via Phabricator via llvm-commits]

rsandifo-arm added a comment.

This approach looks good to me FWIW.  The main advantages as I see it are:

- Treating PSTATE.SM and PSTATE.ZA as orthogonal to the set of implemented features follows the scheme used in the ISA specification, so it's easy to cross-check the two.  The IsSVEEnabled review comment is an example of this.
- It is likely to cope better with any future extensions to SVE or SME: the streaming-compatible, streaming and non-streaming subsets of each ISA extension would not need to be identified individually on the command line or in IR.  Or to put it another way: requiring multiple +streaming-foo flags (and so repeating the “streaming”) feels like specifying the same bit of information multiple times.
- It avoids the risk of combinatorial explosion if we ever need to do something similar for another degree of freedom.

I think my main open questions are:

- Is this backwards-compatible?  Can new tools consume old LLVM IR text and binaries and still work correctly?
- Inline asms should be able to use any instruction, if they set up the environment correctly first.  How do we cope with that without the compiler generating inappropriate instructions?  However, like Sander said off-list, that question applies to both approaches, so it probably doesn't help choose between them.



================
Comment at: llvm/lib/Target/AArch64/AArch64.td:457
+def FeatureSME : SubtargetFeature<"sme", "HasSME", "true",
+    "Enable SME instructions that are valid irrespective of PSTATE.ZA or PSTATE.SM",
+    [FeatureFullFP16, FeatureBF16]>;
----------------
I think from the user's perspective these flags are now normal unqualified "Enable ..” features, i.e. simple “is this feature present?” flags.  The modality is applied on top.


================
Comment at: llvm/lib/Target/AArch64/AArch64.td:466
+// enable/disable any instructions that are not valid in either mode. These features are
+// orthogonal to any of the other features, although some features require both to be
+// enabled in order to be valid.
----------------
I guess “both” means PSTATESM1 and PSTATEZA1.  Might be worth saying that explicitly, since there are three features in play.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:153
+    : Predicate<"Subtarget->hasStreamingCompatibleSVE()">,
+                AssemblerPredicate<(all_of FeatureSVE),
                 "sve or sme">;
----------------
Should this be: SVE | (SME & PSTATESM1) (converted to tabelgen)?  That would follow the semantics of IsSVEEnabled in https://developer.arm.com/documentation/ddi0616/aa/?lang=en .  Same for the other two StreamingCompatible features,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121208