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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the positive feedback and suggestions.

In D128029#3601099 <https://reviews.llvm.org/D128029#3601099>, @lenary wrote:

> I'm very supportive of this, as I wanted it the other day.
>
> I do agree this should not be exposed through `-march` - instead it should be exposed only through `--mattr` which I feel is not quite as user-facing as `-march` and friends.
>
> I wonder whether a better approach to reduce the maintenance burden might be the following:
>
>   def FeatureAll : SubtargetFeature<"all", "IsAll", "true", "Enable all instructions", []>;
>   
>   // or for AArch64
>   class ARMAssemblerPredicate<dag cond, string name=""> : AssemblerPredicate<(any_of FeatureAll, cond), name>;
>
> This might even be something to move into `llvm/include/llvm/Target/Target.td` (I don't know, by changing `class AssemblerPredicate` to `class AssemblerPredicateBase` and then calling what i've defined as `ARMAssemblerPredicate` as `AssemblerPredicate`?

Thanks for the suggestion. It requires some changes to the generic TableGen code which I just did.

As the updated summary says, an encoding may be disassembled differently with different target features.
For many(most?) targets without conflicting encoding, there would be a good user experience improvement but I am not so sure that we may be able to update the generic `AssemblerPredicate`.
In any case, if we ever need to update it, it may be a future day when we have more experience with various targets in llvm updated.

> While I do think updating all could be fixed by process, i.e. when adding a new architecture remember to add to "all", however if there is a way to do this automatically it would be useful to have that in TableGen. Perhaps something like computeAvailableFeatures(). For example setAllFeatures() which just calls Features.set(Feature_...) for all Subtarget features. With perhaps a backend specific option to toggle off specific features if needed.

Due to the code structure, `AArch64MCCodeEmitter::computeAvailableFeatures` is not available to the disassembler. In addition, not all feature bits indicate some instruction set support.
I think for now the most promising approach without imposing too much burden is `class AssemblerPredicateWithAll` as implemented by the latest revision.
We may be able to do something like `computeAvailableFeatures` for disassemblers but that be a larger change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128029