Skip to content

perf(rocprim) Reduce number of kernels generated for several benchmarks and tests - #13092

Merged
stanleytsang-amd merged 4 commits into
developfrom
users/umfranzw/rocprim_build_time_opt
Oct 26, 2026
Merged

stanleytsang-amd merged 4 commits into
developfrom
users/umfranzw/rocprim_build_time_opt

Conversation

@umfranzw

@umfranzw umfranzw commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

We are looking for opportunities to to reduce rocPRIM build time. This change attempts to do that for several benchmark targets.
Jira ID: AIPRIMS-263, EXSWSTRHPC-338

Technical Details

This change introduces two functors for build time optimization of several benchmarks that use lambda functions in a particular context. Here's an example of the optimization for benchmark_device_select:

Several benchmark_device_select benchmarks require a predicate function. Defining that predicate function via a lambda function (within the benchmark's run() implementation) creates this problem:

  • The benchmark struct takes template args. So if we define a predicate function within run() using a lambda, then for each unique instantiation of the benchmark, the lambda predicate function will be given a unique type.
  • The predicate passed into rocprim::select, and its type is passed as a template argument to partition_impl.
  • This means that each unique instantiation of the benchmark will ultimately cause a unique kernel to be generated.
  • Since the predicate-based benchmarks are instantiated with different probabilities, there is a unique kernel generated for each unique probability value, even though the actual kernel code is identical.

Defining the predicates as functors (that take the theshold/flag type as a template parameter) means that the same type is passed into each benchmark instance's call to rocprim::select. This can significantly reduce the number of kernels that need to be generated.

This change implements the above optimization for the following targets:

  • benchmark_device_select
  • benchmark_device_partition

Test Plan

  • Time builds both with and without this change.
  • Ensure that there are no performance regressions at runtime.

Test Result

  • See Jira ticket for build time improvements.
  • Benchmarking for performance regressions is ongoing.

Submission Checklist

This change introduces two functors for build time optimization of several
benchmarks and tests that use lambda functions in a particular context.
Here's an example of the optimization for benchmark_device_select:

Several benchmark_device_select benchmarks require a predicate function. Defining that prodice function via
a lambda function (within the benchmark's run() implementation) creates this problem:
 - The benchmark struct takes template args. So if we define a predicate function within run() using a lambda,
   then for each unique instantiation of the benchmark, the lambda predicate function will be given a unique type.
 - The predicate passed into rocprim::select, and its type is passed as a template argument to partition_impl.
 - This means that each unique instantiation of the benchmark will ultimately cause a unique kernel to be generated.
 - Since the predicate-based benchmarks are instantiated with different probabilities, there is a unique kernel
   generated for each unique probability value, even though the actual kernel code is identical.

Defining the predicates as functors (that take the theshold/flag type as a template parameter) means that the same
type is passed into each benchmark instance's call to rocprim::select. This can significantly reduce the number
of kernels that need to be generated.

This change implements the above optimization for the following targets:
- benchmark_device_select
- benchmark_device_segmented_radix_sort
- benchmark_device_partition
- test_device_segmented_topk
@umfranzw
umfranzw requested review from a team and NB4444 October 5, 2026 15:57
@therock-pr-bot

therock-pr-bot Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/hipccl/hipccl3/rocprim/benchmark/benchmark_device_partition.hpp, projects/hipccl/hipccl3/rocprim/benchmark/benchmark_device_select.hpp; no test file found
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 5, 2026

Copy link
Copy Markdown

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

  • python -m pip install pre-commit
  • pre-commit install
  • pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

There seem to be some performance regressions introduced by these build-time
optimizations, so revert them.
@therock-pr-bot

therock-pr-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@amd-hsong amd-hsong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@amd-jmahovsky amd-jmahovsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but fix the typos.

Claude had a comment about performance that I'll leave as a suggestion:

The huge_1024_f32_f32 benchmarks may now measure something different (benchmark_device_partition.hpp:297, :481; benchmark_device_select.hpp:273).
- Cause: the new functors store the threshold as a full DataType, where the old lambdas captured only a 4-byte float.
- Effect: for custom_huge_type<1024,float,float>, each predicate is now passed into the kernel as a 1 KB argument, most of it the uninitialised data[] array. The three-way
partition benchmark builds two functors (predicate_one and predicate_two, around :557), so it carries about 2 KB where it used to carry 8 bytes.
- Risk: timings for these benchmarks can shift against the baseline even though the predicate logic is unchanged, which weakens the PR's "no performance regression" check.
- Fix: store a float threshold and convert it inside operator():

template struct less_than_op {
float threshold;
bool operator()(const T& v) const { return v < static_cast(threshold); }
};

}

// These two functors exist for build time optimization of predicate-based benchmarks.
// These benchmarks require a predicate function. Defining that prodice function via

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: prodice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, corrected to "predicate". That was a large miscalculation :)

// - This means that each unique instantiation of the benchmark will ultimately cause a unique kernel to be generated.
// - Since the predicate-based benchmarks are instantiated with different probabilities, there is a unique kernel
// generated for each unique probability value, even though the actual kernel code is identical.
// Defining the predicates as functors here (that take the theshold/flag type as a template parameter) means that the same

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: theshold -> threshold

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

@umfranzw

Copy link
Copy Markdown
Contributor Author

LGTM but fix the typos.

Claude had a comment about performance that I'll leave as a suggestion:

The huge_1024_f32_f32 benchmarks may now measure something different (benchmark_device_partition.hpp:297, :481; benchmark_device_select.hpp:273). - Cause: the new functors store the threshold as a full DataType, where the old lambdas captured only a 4-byte float. - Effect: for custom_huge_type<1024,float,float>, each predicate is now passed into the kernel as a 1 KB argument, most of it the uninitialised data[] array. The three-way partition benchmark builds two functors (predicate_one and predicate_two, around :557), so it carries about 2 KB where it used to carry 8 bytes. - Risk: timings for these benchmarks can shift against the baseline even though the predicate logic is unchanged, which weakens the PR's "no performance regression" check. - Fix: store a float threshold and convert it inside operator():

template struct less_than_op { float threshold; bool operator()(const T& v) const { return v < static_cast(threshold); } };

This is a good point, thanks. I will hold off on merging this change until I can run the benchmark before and after the change to see exactly how much of a runtime regression we're talking about.

@umfranzw

Copy link
Copy Markdown
Contributor Author

LGTM but fix the typos.
Claude had a comment about performance that I'll leave as a suggestion:
The huge_1024_f32_f32 benchmarks may now measure something different (benchmark_device_partition.hpp:297, :481; benchmark_device_select.hpp:273). - Cause: the new functors store the threshold as a full DataType, where the old lambdas captured only a 4-byte float. - Effect: for custom_huge_type<1024,float,float>, each predicate is now passed into the kernel as a 1 KB argument, most of it the uninitialised data[] array. The three-way partition benchmark builds two functors (predicate_one and predicate_two, around :557), so it carries about 2 KB where it used to carry 8 bytes. - Risk: timings for these benchmarks can shift against the baseline even though the predicate logic is unchanged, which weakens the PR's "no performance regression" check. - Fix: store a float threshold and convert it inside operator():
template struct less_than_op { float threshold; bool operator()(const T& v) const { return v < static_cast(threshold); } };

This is a good point, thanks. I will hold off on merging this change until I can run the benchmark before and after the change to see exactly how much of a runtime regression we're talking about.

To follow up, benchmarking shows that the runtime differences before and after this change are pretty minimal. With this in mind, I think we can go ahead and merge it.

@stanleytsang-amd
stanleytsang-amd merged commit cf0d592 into develop Oct 26, 2026
247 of 259 checks passed
@stanleytsang-amd
stanleytsang-amd deleted the users/umfranzw/rocprim_build_time_opt branch October 26, 2026 19:11
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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.

4 participants