Skip to content

Use an explicit std::min for byte alignment calculation#1465

Merged
rapids-bot[bot] merged 5 commits intorapidsai:mainfrom
maxwbuckley:patch-1
Oct 28, 2025
Merged

Use an explicit std::min for byte alignment calculation#1465
rapids-bot[bot] merged 5 commits intorapidsai:mainfrom
maxwbuckley:patch-1

Conversation

@maxwbuckley
Copy link
Copy Markdown
Contributor

Use explicit std::min removing compiler ambiguity. This is imported at the top of the file "" and used elsewhere in this file

Use explicit std::min removing compiler ambiguity. This is imported at the top of the file and used elsewhere
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Oct 25, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Oct 25, 2025
@cjnolet cjnolet moved this from Todo to In Progress in Unstructured Data Processing Oct 25, 2025
@benfred
Copy link
Copy Markdown
Contributor

benfred commented Oct 25, 2025

/ok to test a87860c

size_t align_x = alignment_of_2d_array(params.x, params.ldx);
size_t align_y = alignment_of_2d_array(params.y, params.ldy);
size_t byte_alignment = min(align_x, align_y);
size_t byte_alignment = std::min(align_x, align_y);
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.

Silly question: Should we consider converting this to cuda::std::min instead?

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.

(I ask because this is function template in a .cuh file.)

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.

Even though its a .cuh file, the function itself is still a purely host function, so that is not necessary.

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.

Ok, thanks for confirming that this template isn't used in __device__ code. 👍

@benfred
Copy link
Copy Markdown
Contributor

benfred commented Oct 27, 2025

/ok to test 81c3507

Copy link
Copy Markdown
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

👍 Thanks for entertaining the question.

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Oct 28, 2025

/ok to test 324b308

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Oct 28, 2025

/merge

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Oct 28, 2025

Thanks so much for helping us improve the codebase @maxwbuckley. These changes LGTM!

@rapids-bot rapids-bot Bot merged commit 96b6e3c into rapidsai:main Oct 28, 2025
84 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants