Skip to content

std.Target: Introduce Cpu convenience functions for feature tests #23873

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented May 13, 2025

Before:

  • std.Target.arm.featureSetHas(target.cpu.features, .has_v7)
  • std.Target.x86.featureSetHasAny(target.cpu.features, .{ .sse, .avx, .cmov })
  • std.Target.wasm.featureSetHasAll(target.cpu.features, .{ .atomics, .bulk_memory })

After:

  • target.cpu.has(.arm, .has_v7)
  • target.cpu.hasAny(.x86, &.{ .sse, .avx, .cmov })
  • target.cpu.hasAll(.wasm, &.{ .atomics, .bulk_memory })

This is not necessarily the final shape I'd like this API to have; just putting this option up for discussion.

@alexrp alexrp marked this pull request as draft May 13, 2025 16:42
@alexrp alexrp force-pushed the target-features-api branch from 0a90f5d to df3391c Compare May 13, 2025 17:08
Comment on lines +1981 to +1982
pub fn has(cpu: Cpu, comptime family: Arch.Family, feature: @field(Target, @tagName(family)).Feature) bool {
if (family != cpu.arch.family()) return false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit torn between the current behavior here and assert(family == cpu.arch.family()). I like that the current behavior lets you check the family and feature(s) at the same time, but I can also see an argument that asserting might help catch bugs. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick grep shows a bunch of if (is_xyz_cpu) std.Target.xyz.featureSetHas() instances and honestly, I can't imagine a scenario where asserting could be helpful to catch a bug.

@alexrp alexrp force-pushed the target-features-api branch from df3391c to c0355e3 Compare May 13, 2025 17:35
@tensorush
Copy link
Contributor

tensorush commented May 14, 2025

Admittedly, this will sound a bit off-topic, but there's a trivial stylistic nitpick that I have in regards to std/Target.zig, namely, is... functions for determining arch aren't all correctly camelCased:

  • isBSD -> isBsd
  • isAARCH64 -> isAarch64
  • isRISCV -> isRiscV
  • isMIPS -> isMips
  • isMIPS32 -> isMips32
  • isMIPS64 -> isMips64
  • isPowerPC -> isPowerPc
  • isPowerPC32 -> isPowerPc32
  • isPowerPC64 -> isPowerPc64
  • isSPARC -> isSparc

There's also isMinGW, which I'd change to isMinGwLibC to match other abi-checking functions.

Although, I guess these're all breaking changes, technically.
Anyway, sorry if I'm polluting the discussion with an unrelated change.

@alexrp
Copy link
Member Author

alexrp commented May 14, 2025

The problem with those, in my view, is that it's a case where Zig's style clashes with good taste. For example, because we write it as powerpc, riscv, and spirv in snake_case, it should actually be isPowerpc, isRiscv, and isSpirv. I personally really don't like that. isAarch64 is also ugly IMO.

I'm hoping we can just neatly side-step most of this problem with #20690 and #23530.

Before:

* std.Target.arm.featureSetHas(target.cpu.features, .has_v7)
* std.Target.x86.featureSetHasAny(target.cpu.features, .{ .sse, .avx, .cmov })
* std.Target.wasm.featureSetHasAll(target.cpu.features, .{ .atomics, .bulk_memory })

After:

* target.cpu.has(.arm, .has_v7)
* target.cpu.hasAny(.x86, &.{ .sse, .avx, .cmov })
* target.cpu.hasAll(.wasm, &.{ .atomics, .bulk_memory })
@alexrp alexrp force-pushed the target-features-api branch from c0355e3 to e8cba85 Compare May 18, 2025 22:26
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

Successfully merging this pull request may close these issues.

3 participants