Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ARM] Some processors appear to have optional features enabled by default #113008

Open
alexrp opened this issue Oct 19, 2024 · 7 comments
Open

[ARM] Some processors appear to have optional features enabled by default #113008

alexrp opened this issue Oct 19, 2024 · 7 comments

Comments

@alexrp
Copy link
Member

alexrp commented Oct 19, 2024

See this Zig pull request: ziglang/zig#18498 (specifically, this hunk)

It seems that LLVM is enabling features for these processors that are actually optional, per Arm documentation. We're currently working around this in Zig by omitting these features by default in the Zig CPU model/feature data which is sourced from LLVM's.

Would there be interest in a patch to similarly disable these features by default on the LLVM side?

(Note: Please ignore the addition of trustzone to cortex-m85; as discussed in #109770, this was based on a misunderstanding and has since been reverted in Zig.)

@llvmbot
Copy link
Member

llvmbot commented Oct 19, 2024

@llvm/issue-subscribers-backend-arm

Author: Alex Rønne Petersen (alexrp)

See this Zig pull request: https://github.com/ziglang/zig/pull/18498 (specifically, [this hunk](https://github.com/ziglang/zig/pull/18498/files#diff-88edb24013ad7e5428ff29aac0ebba01f919136cfcfe17d806b1892b2af7d065R382-R404))

It seems that LLVM is enabling features for these processors that are actually optional, per Arm documentation. We're currently working around this in Zig by omitting these features by default in the Zig CPU model/feature data which is sourced from LLVM's.

Would there be interest in a patch to similarly disable these features by default on the LLVM side?

(Note: Please ignore the addition of trustzone to cortex-m85; as discussed in #109770, this was based on a misunderstanding and has since been reverted.)

@alexrp
Copy link
Member Author

alexrp commented Oct 30, 2024

cc @davemgreen maybe you have thoughts on this?

@davemgreen
Copy link
Collaborator

I think @smithp35 is away at the moment but might have more to say. @tmatheson-arm and @pratlucas too for the target-parser parts.

AFAIU, the rule in clang, that was agreed with gcc is that:

  • -march=... enables the minimum set of required features for that architecture level. Optional features are added with +xyz.
  • -mcpu=.. enables the maximum optional features for the cpu. They can be disabled with +noxyz.

There are exceptions and some places where that is not followed exactly. It was done this way so that most people get a good default, and people can disable the features they don't have. That isn't to say that llvm needs to follow the same rules, so long as the interface for clang doesn't change.

The features in https://github.com/ziglang/zig/pull/18498/files#diff-88edb24013ad7e5428ff29aac0ebba01f919136cfcfe17d806b1892b2af7d065R382-R404 are either

  • fp
  • dsp
  • mve
  • pacbti.

pacbti is considered a benign architecture feature, as it only enables assembly/disassembly. The actual codegen changes are controlled with other options. fp is complicated in clang/gcc by the -mfpu option and -mfloat-abi depending on the abi. In clang it essentially defaults to -mcpu=auto from gcc.

GCC docs are available at https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html. I guess I would recommend the same scheme for other frontends. Needless differentiation between the frontends does not sound ideal.

@alexrp
Copy link
Member Author

alexrp commented Nov 1, 2024

I'm only concerned with the processor definitions in LLVM specifically. If we can change those to only imply the minimum set of features there, while leaving them unchanged at the Clang level, I agree that would be ideal.

@smithp35
Copy link
Collaborator

smithp35 commented Nov 4, 2024

I agree with @davemgreen on the rules of thumb for CPU features.

I'm hesitant to recommend changing the LLVM defaults, even if clang remains stable, without wider consultation via an RFC. For example it would affect other front-ends built on top of LLVM like Rust as well.

One possible solution, and I'd need to defer to @tmatheson-arm and @pratlucas for how practical it would be, is for us separate out what feature is optional for a CPU or not, and whether LLVM enables it by default for a CPU. That way LLVM can act as a machine readable technical reference manual (TRM) for the CPU, without affecting the default option it presents to front-ends.

@nikic
Copy link
Contributor

nikic commented Nov 7, 2024

I'm hesitant to recommend changing the LLVM defaults, even if clang remains stable, without wider consultation via an RFC. For example it would affect other front-ends built on top of LLVM like Rust as well.

FWIW, Rust ran into the same issue, previously reported at #90365 and also discussed at rust-lang/rust#125033. As far as I understand it, the preference on the Rust side would be for LLVM to use the minimum guaranteed set of target features.

@workingjubilee
Copy link
Contributor

workingjubilee commented Nov 8, 2024

GCC docs are available at https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html. I guess I would recommend the same scheme for other frontends. Needless differentiation between the frontends does not sound ideal.

Frontends that would like to have sounder defaults for their CLI options, and are strongly asked by many of their library developers and target maintainers to do so, are not engaging in "needless" differentiation.

I do not expect you to agree on what is "necessary" versus "unnecessary", but the conversations I've had with Rust's embedded ARM devs and maintainers strongly suggest that they are considering PRing a workaround to this issue on rustc's level. By the sounds of it, this might be primarily by completely abandoning the target-cpu definitions that LLVM uses for that architecture and rerolling them, starting only with a tune-cpu setting and building up features on top. This seems to become even more likely if LLVM continues holding the position that everyone has to use the maximal featureset. This would introduce even more divergence from LLVM's current scheme. That would not be due to the need being addressed, but simply due to errors when the features don't actually match the CPU's feature set, minimal or maximal. Then duplicate work would happen, and then... then THAT would be truly unnecessary, I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants