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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked 2 inline comments as done.
sdesmalen added a comment.

Hi @rsandifo-arm, thanks for your comments!

> Is this backwards-compatible? Can new tools consume old LLVM IR text and binaries and still work correctly?

This approach is backwards-compatible, because the `+pstate-*` features are added 'automatically' when LLVM calls the constructor for the AArch64 Subtarget, which is the state that LLVM queries for any target-features/capabilities. It's worth pointing that once we add these flags, any bitcode created by LLVM will always have these features set in the IR, so it won't be easy to change/remove these flags afterwards.

> 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.

While I believe this is a separate concern and should be addressed in a separate patch, I think we can implement this with an extra LLVM-internal feature flag. We could add some `-force-assemble-pstate-sm1` feature that doesn't impact the `hasSME()`- or `hasPSTATESM1()`-questions the compiler may ask for example when generating code and is therefore only used by the assembler/disassembler. For example for a streaming-compatible function which gets the attributes `-pstate-sm0,-pstate-sm1,-pstate-za1`, we could let Clang add `+force-assemble-pstate-sm0,+force-assemble-pstate-sm1,+force-assemble-pstate-za1` to force any inline-asm code to be assembled regardless of PSTATE.SM/ZA, even though for any other purposes LLVM will assume PSTATE.SM/ZA are not available.



================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:153
+    : Predicate<"Subtarget->hasStreamingCompatibleSVE()">,
+                AssemblerPredicate<(all_of FeatureSVE),
                 "sve or sme">;
----------------
rsandifo-arm wrote:
> 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,
Yes it should, although there seems to be a problem with TableGen in trying to express this.
The assembler behaves as expected, with the exception of:

  $ echo "ptrue p0.b, pow2" | ./bin/llvm-mc -mattr=+sme,-pstate-sm0,-pstate-sm1 -
        .text
        ptrue   p0.b, pow2

Where I had expected: `<stdin>:1:1: error: instruction requires: sve or sme`

I don't think this is a problem though, since `pstate-sm0` and `pstate-sm1` are not user-facing features, and also because the `AssemblerPredicate` relates only to the assembler.
Other code in LLVM will rely on the `hasStreamingCompatible<Feature>()` functions which return the correct result, so there's no chance of the compiler assuming it can use any instructions that it shouldn't. That's why I'm not too worried about having this as a FIXME for now.

Also as you mentioned, we want inline-asm to handle all instructions regardless of PSTATE.SM.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121208