Open armandomontanez opened 2 months ago
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.
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.
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.
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.
A few other ideas that have come up in side discussions:
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"],
)
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:
string_flag
s, 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.config_setting
s 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 https://github.com/bazelbuild/platforms/issues/104#issuecomment-2364287290. I think Android rightly uses -march
because there's a need for wider portability for the final binaries.
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.