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

[discussion] Decide whether or not to express FPU nuances for @platforms//cpu:armv*-m* constraints #100

Open
armandomontanez opened this issue Aug 15, 2024 · 6 comments

Comments

@armandomontanez
Copy link

We may want to avoid expressing FPU extensions in ARM Cortex-M CPU constraints.

Proposal

Refrain from extending the set of @platforms//cpu:armv*-mf constraints, and annotate the existing constraint with a warning that it is kept around for compatibility reasons and shouldn't be carried forwards as a pattern.

Why?

ARMv7e-M + FPU is relatively ambiguous:
Cortex-M4 (ARMv7e-M) optionally supports FPU instructions via the FPv4-SP extension
Cortex-M7 (ARMv7e-M) optionally supports FPU instructions via the FPv5-SP-D16-M or FPv5-DP-D16-M extensions

These aren't the only optional extensions for Cortex-M processors either, so properly fully enumerating the extensions is a bit of a hassle when it comes to basic CPU constraints. It's a bit like having @platforms//cpu:x86_64-AVX512. It's likely best to leave these nuances to separate constraints that add dimensions rather than trying to enumerate every configuration across a single dimension. I don't necessarily think we should prescribe a solution for expressing architecture extensions, but I do think we should consider annotating the existing @platforms//cpu:armv7e-mf with a warning that it shouldn't be replicated but will be retained for backwards compatibility.

@katre
Copy link
Member

katre commented Aug 19, 2024

This sounds very reasonable to me, but no one on the team has any deep knowledge about ARM and how these things are handled. I'm going to send this to the Slack to see if anyone there has an opinion, and if we don't hear back this sounds like a reasonable approach.

@aiuto
Copy link
Contributor

aiuto commented Aug 19, 2024

Agreed. The various features which pop up in each architecture should be listed as individual constraints.
We got to the state we are in by trying to quickly acomodate existing users.

I would be happy seeing cpu/arm/BUILD, cpu/x86/BUILD, cpu/riscv/BUILD, ... and so on to hold the names of platform specific features. Then people can build up their own hardware specific platforms from those.
What you still have, however, is an education battle with users who don'd try to select on the best constraint. If a piece of hardware has 3 feature constraints specified in the platform, I'll bet that a significant percentage of people will do a select wrong. I don't have time to go into a sanitized example today, but people are too used to using overly broad proxy signals for the feature they really care about.

@armandomontanez
Copy link
Author

I like the idea of having @platforms//cpu/*/BUILD subdirectories to express other dimensions, though I'd definitely be wary about hastily defining those. I suspect most of the time the least error-prone answer is to let the finer granularity be defined by end-users.

@mattyclarkson
Copy link

I agree that @platforms//cpu/*/BUILD with CPU specific contraints that users can build up into specific platforms would make sense.

I'll attempt to find a relevant expert within Arm to advise.

@armandomontanez
Copy link
Author

armandomontanez commented Oct 24, 2024

A few other ideas that have come up in side discussions:

Create base platforms for projects to use

Since expressivity gets a tad more complicated with these, we could create platforms that projects could inherit from:

platform(
    name = "cortex-m4",
    constraint_values = [
        "@platforms//cpu/arm:cortex-m4",
    ],
)

platform(
    name = "cortex-m4+fpv4-d16",
    constraint_values = [
        "@platforms//cpu/arm:supports_fpv4_d16",
    ],
    parents = [":cortex-m4"],
)

Build compiler flags based on these

Another idea I was sitting on was constructing a -mcpu flag based on these. The "how" isn't the most obvious for a couple reasons:

  1. It's pretty easy to get the values we want if we represent CPU and FPU as string_flags, but those make it harder to use the flags as you would a constraint_value since you can't list them in target_compatible_with directly. Also, exposing these as string flags makes usage more error-prone.
  2. Representing CPU and FPU flavors as config_settings makes it harder to construct a -mcpu=aaa+bb string dynamically. It's possible, but not particularly elegant.

At the end of the day, what I'd like is something like:

cc_args(
    name = "mcpu",
    args = ["-mcpu={cpu}+{fpu}"],
    format = {
        "cpu": "@platforms//cpu:cpu",
        "fpu": "@platforms//cpu/arm:fpu",
    },
)

But implementing that might not be quite so trivial.

Also note that I'm using -mcpu here rather than -march for the reasons listed in #104 (comment). I think Android rightly uses -march because there's a need for wider portability for the final binaries.

@mattyclarkson
Copy link

The general concensus internally at Arm (who are not huge users of Bazel) is "do whatever the compilers do".

@armandomontanez basing constraints on what the compilers can enable with -mcpu aligns with this. I like your above suggestion.

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

No branches or pull requests

4 participants