Skip to content

HIP/CUDA Gen: Use correct block size bounds in non-tensor dynamic dispatch calculations#1993

Open
zatkins-dev wants to merge 3 commits into
mainfrom
zach/gen-block-size-fix
Open

HIP/CUDA Gen: Use correct block size bounds in non-tensor dynamic dispatch calculations#1993
zatkins-dev wants to merge 3 commits into
mainfrom
zach/gen-block-size-fix

Conversation

@zatkins-dev

Copy link
Copy Markdown
Collaborator

Purpose:

Removes hard coded constants for block size bounds with dynamically queried values for both HIP and CUDA gen backend non-tensor kernels.

Also, fixes a bug in the field reuse code in HIP.

Closes:

LLM/GenAI Disclosure:

None.

By submitting this PR, the author certifies to its contents as described by the Developer's Certificate of Origin.
Please follow the Contributing Guidelines for all PRs.

@zatkins-dev
zatkins-dev requested a review from jeremylt July 17, 2026 17:31
@zatkins-dev zatkins-dev changed the title Zach/gen block size fix HIP/CUDA Gen: Use correct block size bounds in non-tensor dynamic dispatch calculations Jul 17, 2026
@zatkins-dev zatkins-dev self-assigned this Jul 17, 2026
@zatkins-dev

Copy link
Copy Markdown
Collaborator Author

See #1994 for my thoughts on how to automatically catch these failures moving forward.

@zatkins-dev

Copy link
Copy Markdown
Collaborator Author

Also, note that we have to use hipModuleOccupancyMaxPotentialBlockSize, not hipOccupancyMaxPotentialBlockSize (like the CUDA equivalent cudaOccupancyMaxPotentialBlockSize), as HIP requires the functions passed to the latter to be compiled in advance using hipcc/hipcxx.

@zatkins-dev

Copy link
Copy Markdown
Collaborator Author

Also, should we use a more sophisticated algorithm for the HIP block and grid sizes, like we do for CUDA? It seems like the limitation was in not knowing the optimal minimum grid size and block sizes, which we now have access to.

@jeremylt

Copy link
Copy Markdown
Member

We need to do profiling here probably, because I think this has serious perf implications? @nbeams and @YohannDudouit know way better than I

@zatkins-dev

Copy link
Copy Markdown
Collaborator Author

We need to do profiling here probably, because I think this has serious perf implications?

FWIW -- The current code always falls back for Nitsche operators in Ratel, regardless of quadrature/solution order. This is a sizable perf improvement just by virtue of actually supporting gen in those cases. Not sure about general non-tensor, but I imagine this will fix some issues there too.

@jeremylt

Copy link
Copy Markdown
Member

Good point. I think a combo of profiling and expert opinion probably is still appropriate to make sure we are appropriately slicing the available resources?

cuda_data->device_prop.maxThreadsDim[2], cuda_data->device_prop.warpSize, block, &grid));
} else {
CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(512 / data->thread_1d, 1));
CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(max_threads_per_block / data->thread_1d, 1));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We could also leave the cap on 512 by doing this:

Suggested change
CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(max_threads_per_block / data->thread_1d, 1));
CeedInt elems_per_block = CeedIntMin(cuda_data->device_prop.maxThreadsDim[2], CeedIntMax(CeedIntMin(max_threads_per_block, 512) / data->thread_1d, 1));

That would ensure the previous behavior for the standard non-tensor elements while preventing the elems_per_block from being too high for the weird cases.

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.

2 participants