This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add target feature "all"
ClosedPublic

Authored by MaskRay on Jun 16 2022, 10:07 PM.

Details

Summary

This is used by disassemblers: llvm-mc -disassemble -mattr= and llvm-objdump --mattr=.
The main use case is for llvm-objdump to disassemble all known instructions
(D128030).

In user-facing tools, "all" is intentionally not supported in producers:
integrated assembler (.arch_extension all), clang -march (-march=armv9.3a+all).
Due to the code structure, llvm-mc -mattr=+all llc -mattr=+all are not
rejected (they are internal tool). Add llvm/test/CodeGen/AArch64/mattr-all.ll
to catch behavior changes.

AArch64SysReg::SysReg::haveFeatures: check FeatureAll to print
AArch64SysReg::SysReg::AltName for some system registers (e.g. ERRIDR_EL1, RNDR).

AArch64.td: add AssemblerPredicateWithAll to additionally test FeatureAll.
Change all AssemblerPredicate (except UseNegativeImmediates) to AssemblerPredicateWithAll.

utils/TableGen/{DecoderEmitter,SubtargetFeatureInfo}.cpp: support arbitrarily
nested all_of, any_of, and not.

Note: A predicate supports all_of, any_of, and not. For a target (though
currently not for AArch64) an encoding may be disassembled differently with
different target features.
Note: AArch64MCCodeEmitter::computeAvailableFeatures is not available to
the disassembler.

Diff Detail

Event Timeline

MaskRay created this revision.Jun 16 2022, 10:07 PM
MaskRay requested review of this revision.Jun 16 2022, 10:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 10:07 PM
MaskRay edited the summary of this revision. (Show Details)Jun 16 2022, 11:10 PM
c-rhodes added inline comments.
llvm/lib/Target/AArch64/AArch64.td
558–566

I think this will introduce a bit of a maintenance burden and I can see new features being missed, could this be generated by tablegen?

Should https://reviews.llvm.org/D128030 be marked in phab as a child to this patch, to show the dependency?

nickdesaulniers added a comment.EditedJun 17 2022, 9:26 AM

Honestly, I think this feature list should just exist in the disassembler. Is this generally useful when building code? I don't think so; its meaning and ABI will change over time as more extensions are added to the architecture.

llvm/lib/Target/AArch64/AArch64.td
558–566

Or perhaps an assertion somewhere if the possible feature list grows to alert this needs to be updated?

MaskRay added a comment.EditedJun 17 2022, 10:19 AM

Honestly, I think this feature list should just exist in the disassembler. Is this generally useful when building code? I don't think so; its meaning and ABI will change over time as more extensions are added to the architecture.

It is generally not useful when building code, so I have emphasized that clang -march= does not support all. I think such a feature list does not exist in the disassembler.
There is some maintenance cost, but lacking some stuff when new target features emerge does not matter: people running llvm-objdump may notice and we can add new target features to "all".

I think for some targets there are conflicting encodings. Then the "all" feature may probably need to pick the mostly likely instruction sets users expect.

Do we have equivalent "all" features for other architectures? It might be nice to have an interface architectures can provide to list a list of all possible features. Then we have one place to keep up to date, then other call sites can share that code. That might help us notice sooner when the "all" list is missing a new addition if there's more than one user, eventually.

MaskRay added a comment.EditedJun 17 2022, 10:44 AM

Do we have equivalent "all" features for other architectures? It might be nice to have an interface architectures can provide to list a list of all possible features. Then we have one place to keep up to date, then other call sites can share that code. That might help us notice sooner when the "all" list is missing a new addition if there's more than one user, eventually.

PowerPC has -mcpu=future (see D127824), which is more or less "all" features.
Note that some targets may have conflicting instruction sets, at which point an all feature will not exist. (IIRC PowerPC's Signal Processing Engine extension falls into this case, so -mcpu=future likely cannot decode some instructions).
We may have several choices of all: (a) great common divisor (b) all target features user mostly likely expect.

In llvm/lib/Target/Mips/MipsInstrInfo.td, there are assembler predicates using not operator, e.g. AssemblerPredicate<(all_of (not FeatureMips3))>. Apparently there are conflicting instruction sets.

MaskRay added inline comments.Jun 17 2022, 10:47 AM
llvm/lib/Target/AArch64/AArch64.td
558–566

Explained in a main comment:

but lacking some stuff when new target features emerge does not matter: people running llvm-objdump may notice and we can add new target features to "all".

This controversy is the main point that we do not add a user-facing (e.g. clang -march) support for all.

MaskRay edited the summary of this revision. (Show Details)Jun 21 2022, 12:08 PM
srhines added inline comments.
llvm/lib/Target/AArch64/AArch64.td
558–566

I'm inclined to believe that this is a reasonable maintenance burden, but perhaps someone from ARM should weigh in here. It's in precisely the best place to be noticed when adding a new AArch64 subtarget/feature. Ultimately this is being added to support improved experiences for llvm-objdump, and it does that. If there are missing subtargets/features in the future, it seems relatively straightforward to add them here.

The general idea sounds useful to me, but FeatureTME seems to be missing (as one I randomly tried). I've added some more reviewers who might have more of an opinion on the best way to structure it.

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?

I definitely agree that there should be the functionality in the disassembler. By default Arm's proprietary toolchain operates in this mode, for want of a better word, we called it universal disassembly. There is an option for architecture aware disassembly by giving it a CPU. For most of Arm (some unfortunate overlap with some mutually exclusive M-profile and A profile extensions) and all of AArch64 there are no overlapping encodings so universal tends to work well.

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.

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.

MaskRay updated this revision to Diff 439166.Jun 22 2022, 2:45 PM
MaskRay edited the summary of this revision. (Show Details)

Use AssemblerPredicateWithAll

Thanks for the positive feedback and suggestions.

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.

MaskRay updated this revision to Diff 439192.Jun 22 2022, 3:42 PM
MaskRay marked 3 inline comments as done.
MaskRay edited the summary of this revision. (Show Details)

Don't change UseNegativeImmediates (though it doesn't matter anyway because the all_of (not ...) is used in a way that whether FeatureAll is tested does not matter)

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

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.

llvm/test/MC/AArch64/directive-arch_extension.s
99

Can you move this test into directive-arch_extension-negative.s, where we already expect the directive to fail and emit errors?

MaskRay updated this revision to Diff 439669.Jun 24 2022, 1:52 AM
MaskRay edited the summary of this revision. (Show Details)

Move isValidSysReg's FeatureAll change to llvm/lib/Target/AArch64/Utils/AArch64BaseInfo.h

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

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.

Thanks. Changing SysAlias::haveFeatures is better. Updated.

lenary accepted this revision.Jun 27 2022, 11:58 AM

I'm happy with this (and I satisfy "someone from Arm" in the comment above). I think others are too, but maybe wait for another @peter.smith comment before landing?

In reply to @nickdesaulniers comment, we can probably move some of the non-aarch64-specific tablegen changes into the target-independent tablegen in a follow-up commit, which will also need IsAll to be moved to the subtarget base class. That seems routine and easy, I hope.

This revision is now accepted and ready to land.Jun 27 2022, 11:58 AM
Matt added a subscriber: Matt.Jun 28 2022, 2:34 PM

No objections from me. I'm not an expert in this area so I've got a few questions/suggestions based on trying to understand the changes.

llvm/test/TableGen/AsmPredicateCombining.td
53

IIUC code has been added to be able to nest any_of and all_of could we add a test for that case.

llvm/utils/TableGen/DecoderEmitter.cpp
1231

Could you explain what Outer is precisely. To me the name implies it is meant to be true only when there is an expression with parens. For example any_of(x, y, z) when processing y assuming it isn't another any_of or all_of then I would expect Outer to be false (an inner if you like). It looks like Outer is set when processing an all_of or any_of with more than 1 parameter. If I'm right it might lead to more parentheses than we'd need, but this isn't a problem for correctness.

1252

One thing I haven't been able to work out is whether it is possible for Paren to be true and D->Args() > 0 so that we get OS << '(' then OS << LS which I assume would end up as (&& or (|| this doesn't look to be happening in the test cases, but I can't quite rule it out from the code here.

llvm/utils/TableGen/SubtargetFeatureInfo.cpp
154

Worth adding a false /* Outer */?

MaskRay updated this revision to Diff 441072.Jun 29 2022, 10:27 AM
MaskRay marked 5 inline comments as done.

address comments

llvm/test/MC/AArch64/directive-arch_extension.s
99

Done. For many tests, I think using one file for both positive and negative tests may be clearer. Since directive-arch_extension-negative.s exists, I'll not change it in this patch :-)

llvm/utils/TableGen/DecoderEmitter.cpp
1231

Thanks for the comments. Added a comment.

Outer is simplified from a more complex recursive expression printer.

a * (b + c)
    ------- current recursive call

For a recursive call where OuterPrecedence indicates *, if the inner expression uses an operator (e.g. +) which binds less than the outer operator, a pair of () is needed.

In our case, for the following expression

a && (b && c)
     ________

we print a pair of superfluous parentheses. But for most cases, the new algorithm prints fewer () than before.

1252

ListSeparator does not print a delimiter for the first << call. (&& or (|| cannot happen.

llvm/utils/TableGen/SubtargetFeatureInfo.cpp
154

I pick /*ParenIfBinOp=*/false. This style is allowed by both clang-format and clang-tidy's bugprone-argument-comment check.

MaskRay updated this revision to Diff 441074.Jun 29 2022, 10:30 AM

minor adjustment

peter.smith accepted this revision.Jun 30 2022, 6:02 AM

Thanks for the updates, LGTM too.

MaskRay edited the summary of this revision. (Show Details)Jun 30 2022, 10:37 AM
This revision was landed with ongoing or failed builds.Jun 30 2022, 10:38 AM
This revision was automatically updated to reflect the committed changes.

Thanks for implementing this!

I've been trying to use it in lldb's disassembler and found that for the "predres" extension +all doesn't work as expected.

$ ./bin/llvm-mc --triple aarch64 --assemble -o - -mattr=+all --show-encoding <<< "cfp rctx, x0"
        .text
<stdin>:1:5: error: CFPRCTX requires: predres
cfp rctx, x0
    ^

I think I know what causes this and it's that AArch64AsmParser::parseSysAlias uses the feature bits from the SubTargetInfo. This is before "+all" has set all the other feature bits. It should be using the feature bits stored in the AsmParser, so I'm looking at changing it to do that.

Anything that isn't a sys instruction alias is fine.

Turns out there was a much simpler soloution: https://reviews.llvm.org/D129147