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

Mark the cache/perf counter experiments as x86-64 only. #23

Merged
merged 2 commits into from
Oct 21, 2023

Conversation

ondrasej
Copy link
Collaborator

The experimental code depends on x86-64 specific intrinsics, and breaks the build on Apple silicon and other non-x86 platforms.

The experimental code depends on x86-64 specific intrinsics, and breaks the
build on Apple silicon and other non-x86 platforms.
Copy link
Collaborator

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

LGTM (not too familiar with the bazel specifics though). There's also the runtime component of these tests (access to performance counters) that cause them to fail at test time in specific configurations, but I'm hoping to get to that at some point in a future patch.

@boomanaiden154
Copy link
Collaborator

Also looks like this hasn't been rebased on top of a recent commit in main which is why there is the double CI jobs/some of the jobs are failing.

@ondrasej
Copy link
Collaborator Author

LGTM (not too familiar with the bazel specifics though). There's also the runtime component of these tests (access to performance counters) that cause them to fail at test time in specific configurations, but I'm hoping to get to that at some point in a future patch.

Good point, perhaps we should limit them to x86-64 & Linux (I don't suppose the perf counter access in googlebenchmark is platform-independent).

Also looks like this hasn't been rebased on top of a recent commit in main which is why there is the double CI jobs/some of the jobs are failing.

That recent commit is #22? That is what I see as the top of main - maybe the recent commit got lost somewhere?

@boomanaiden154
Copy link
Collaborator

Nevermind. Wasn't reading things correctly. Half of them are push jobs and half of them are pull_request jobs because I don't have the triggers set up correctly. I'll get a patch up to fix that.

Given that, the Python formatting errors are probably valid too. Not exactly sure what changed there though to cause them to start failing.

@ondrasej ondrasej merged commit 320584f into main Oct 21, 2023
11 of 13 checks passed
@ondrasej ondrasej deleted the fix-arm-build branch October 21, 2023 21:07
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