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

Prebuild the benchmark binary with -O3 on Android #532

Merged
merged 2 commits into from
Oct 14, 2020

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Oct 14, 2020

What do these changes do?

This PR makes sure that the Android benchmarking binary get's built with all optimizations enabled. Previously we also set -O3 in the Raspberry PI configs, I moved this now the the release built in the hope to slightly speedup ARM CI.

Benchmark Results

I didn't see any measurable difference on my system, but the generated binary has a slightly different size, so it is definitely doing something.

@lgeiger lgeiger added the internal-improvement Internal Improvements and Maintenance label Oct 14, 2020
@lgeiger lgeiger requested a review from a team October 14, 2020 11:30
@lgeiger
Copy link
Member Author

lgeiger commented Oct 14, 2020

Looks like this change enabled debug checks for the ARM unit tests which now fail at:

// Ruy supports collecting column sums during the packing process, which we
// have no need for.
RUY_DCHECK_EQ(packed_matrix->sums, nullptr);

I think #529 touches the same code, I can try and debug what's going on after that has been merged.

@AdamHillier
Copy link
Contributor

Ah yes I had long suspected that debug checks were for some reason disabled, and this confirms it haha. That specific check by the way was added by me and I'm pretty sure it can be removed.

@lgeiger
Copy link
Member Author

lgeiger commented Oct 14, 2020

That specific check by the way was added by me and I'm pretty sure it can be removed.

I'm fine with removing the check.

The matrices are prepared in:

ruy::PreparePackedMatrices(ruy_ctx, &bgemm_trmul_params);

Which sets the sums here. Do you think we should remove this instead, or would this break things?

@AdamHillier
Copy link
Contributor

Do you think we should remove this instead, or would this break things?

I'm not sure what the impact of removing that call would be, so for now I would probably just remove the debug check.

@lgeiger lgeiger requested a review from AdamHillier October 14, 2020 13:17
@AdamHillier AdamHillier merged commit ec2de3f into master Oct 14, 2020
@AdamHillier AdamHillier deleted the copt-benchmarking branch October 14, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-improvement Internal Improvements and Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants