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

GH-43687: [C++] Compute: fix register kernel SimdLevel for AddMinMax512AggKernels #43704

Merged

Conversation

mapleFU
Copy link
Member

@mapleFU mapleFU commented Aug 15, 2024

Rationale for this change

See #43687

What changes are included in this PR?

Change Registered AVX2 to AVX512

Are these changes tested?

No

Are there any user-facing changes?

maybe bugfix

Copy link

⚠️ GitHub issue #43687 has been automatically assigned in GitHub to PR creator.

Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

+1

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 15, 2024
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

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

I'm approving and we should merge this, but I'm working on a PR that removes the SimdLevel template parameter alltogether as there is no need to runtime dispatch different SIMD levels when we are relying on compiler auto-vectorization.

Discussion here: #7871

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Aug 15, 2024
@mapleFU
Copy link
Member Author

mapleFU commented Aug 15, 2024

Will merge this firstly

@mapleFU mapleFU merged commit 2e434da into apache:main Aug 15, 2024
41 of 42 checks passed
@mapleFU mapleFU removed the awaiting merge Awaiting merge label Aug 15, 2024
@mapleFU mapleFU deleted the fix-simd-level-in-aggregate-basic-avx512 branch August 15, 2024 16:11
@ursabot
Copy link

ursabot commented Aug 15, 2024

Supported benchmark command examples:

@ursabot benchmark help

To run all benchmarks:
@ursabot please benchmark

To filter benchmarks by language:
@ursabot please benchmark lang=Python
@ursabot please benchmark lang=C++
@ursabot please benchmark lang=R
@ursabot please benchmark lang=Java
@ursabot please benchmark lang=JavaScript

To filter Python and R benchmarks by name:
@ursabot please benchmark name=file-write
@ursabot please benchmark name=file-write lang=Python
@ursabot please benchmark name=file-.*

To filter C++ benchmarks by archery --suite-filter and --benchmark-filter:
@ursabot please benchmark command=cpp-micro --suite-filter=arrow-compute-vector-selection-benchmark --benchmark-filter=TakeStringRandomIndicesWithNulls/262144/2 --iterations=3

For other command=cpp-micro options, please see https://github.com/voltrondata-labs/benchmarks/blob/main/benchmarks/cpp_micro_benchmarks.py

@felipecrv
Copy link
Contributor

@ursabot please benchmark command=cpp-micro --suite-filter=arrow-acero-aggregate-benchmark --benchmark-filter=MinMaxKernel* --iterations=3

@ursabot
Copy link

ursabot commented Aug 15, 2024

Benchmark runs are scheduled for commit cb073ed. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

Copy link

Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit cb073ed.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

Copy link

After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 2e434da.

There were 3 benchmark results indicating a performance regression:

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Successfully merging this pull request may close these issues.

5 participants