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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I'll admit that my SME knowledge is sketchy but I cannot help but think you're causing yourself problems by tying parts of the code generator together that don't need to be tied.  As I see it there are two distinct problems that need to be solved:

1. SME enables a set of instructions that partially overlap with NEON and SVE.
2. SME has runtime modes that further restricts what instructions are available.

Both of these can be represented using feature flags but that doesn't mean they require symmetrical coverage.

The feature flags required for (1) are required to protect instruction definitions and usage (i.e. assembler support, isel patterns...).  These features are user visible as used by `-march=` and similar options.

The feature flags [1] used for (2) exist to allow pre isel code generation to make informed choices as to what builtins/transformations are safe to use.  When used correctly there should never be an instance where an isel pattern is used in error.  If a code path really wants to emit instructions that sit outside of feature (2) (inline asm for example) it seems simpler to assume they know what they're doing and let them.  Sure a bug might exist where this logic is wrong but treating (1) and (2) the same just means a runtime SIGILL is replaced with a compiler crash and I don't think the extra complexity is worth it.  Especially when we're further down the road and fighting ten more years of new feature options.

I personally think the source of many of these problems will be much higher in the stack.  For example, when it comes to the NEON and SVE builtins I figure we'll want to detect invalid calls within clang based on the c/c++ function decorations we'll need anyway.  That way the user will see a nice diagnostic rather than a compiler crash or runtime failure.

[1] I use feature flags here as that's what you've implemented, but again here there's a choice.  We can use whatever LLVM function decoration is most appropriate.  I'm not saying a feature flag is wrong, in fact it seems clean enough, I'm just highlighting you've got options that should be considered as part of the wider SME support within LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121208