[PATCH] D128029: [AArch64] Add target feature "all"

Thu Jun 23 01:57:24 PDT 2022

lenary added a comment.

I didn't realise you couldn't nest `any_of` and `all_of` before this patch. Thanks for catching that.

In D128029#3601135 <https://reviews.llvm.org/D128029#3601135>, @peter.smith wrote:

> Main reason for mentioning that is to ask the question; is -mattr=all too low level a concept to model universal disassembly on the command line? I'm thinking it is fine for llvm-mc but perhaps llvm-objdump could do with something a bit more high-level.

I'm not sure how much `-mattr` is a part of the public/backwards-compatible command-line interface for llvm-objdump. I think we could absolutely include an option for this in a command-line interface we commit to not changing, usually the target-specific options are behind `llvm-objdump -M`, which I believe should have enough access to the subtarget to be able to enable features.

The only other suggestion I have apart from the one below is whether we should change the definition of `SysAlias::haveFeatures` in `llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h` to also check for FeatureAll - I think how those tables are currently structured, they don't use AssemblyPredicates, they use feature bitsets directly, which is maybe not optimal. If you want to leave this refactoring for later, that would make sense to me too.

================
Comment at: llvm/test/MC/AArch64/directive-arch_extension.s:99
+
+.arch_extension all
+// ERR: :[[#@LINE-1]]:17: error: unknown architectural extension: all
----------------
Can you move this test into `directive-arch_extension-negative.s`, where we already expect the directive to fail and emit errors?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128029