cuda: Remove compile-time dependence on maximum number of points per elements#1990
cuda: Remove compile-time dependence on maximum number of points per elements#1990zatkins-dev wants to merge 1 commit into
Conversation
zatkins-dev
force-pushed
the
zach/cuda-remove-max-num-points
branch
from
b1e999b to
b3eae8e
Compare
|
I'll check more tomorrow or Friday, but at a blush it seems reasonable. HIP coming too? And did you check in Ratel when we know the max number of points changed? Other thought - we should profile as I thought the data access optimization of knowing the indexing stride was noticeable, but I can recall why I thought that. |
|
I can profile and add HIP next week -- moving this weekend and wanted to get this proof of concept up for review. |
Technically we trash the operators in Ratel whenever things change -- it will be difficult to fully test until we have the operator rebuild interface. |
I would expect that the real impact is that the indexing stride is uniform across the thread block. I also think the loop unrolling for batches of points (which is definitely broken by this) would be a bigger impact. We can probably recover perf via manual unrolling or by using a fixed batch size instead. |
|
|
||
| // Check padded to uniform number of points per elem | ||
| for (CeedInt i = 1; i < num_elem; i++) max_num_points = CeedIntMax(max_num_points, num_points[i]); | ||
| data->num_points = max_num_points; |
There was a problem hiding this comment.
I'd probably call this data->max_num_points, dunno why I named it the way I did before
| case CEED_EVAL_INTERP: { | ||
| void *interp_args[] = {(void *)&num_elem, &data->d_chebyshev_interp_1d, &data->d_points_per_elem, &d_x, &d_u, &d_v}; | ||
| const CeedInt block_size = CeedIntMin(CeedIntPow(Q_1d, dim), max_block_size); | ||
| void *interp_args[] = {(void *)&num_elem, (void *)&data->num_points, &data->d_chebyshev_interp_1d, &data->d_points_per_elem, &d_x, &d_u, &d_v}; |
There was a problem hiding this comment.
we should see if we have (or should establish) a consistent pattern for the inputs to these kernels
| case CEED_RESTRICTION_POINTS: { | ||
| CeedInt max_num_points; | ||
|
|
||
| CeedCallBackend(CeedElemRestrictionGetMaxPointsInElement(rstr, &max_num_points)); |
There was a problem hiding this comment.
this should never change for the restriction so we can save the effort of recomputing in on every application here
| CeedCallBackend(CeedOperatorAtPointsGetPoints(op, NULL, &vec)); | ||
| CeedCallBackend(CeedOperatorAtPointsGetPoints(op, &rstr_points, &vec)); | ||
| CeedCallBackend(CeedVectorGetArrayRead(vec, CEED_MEM_DEVICE, &data->points.coords)); | ||
| CeedCallBackend(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &data->points.max_num_points)); |
There was a problem hiding this comment.
this should only change if the restriction changes, see the other comment about avoiding re-computing
| CeedCallBackend(CeedOperatorAtPointsGetPoints(op, &rstr_points, &vec)); | ||
| CeedCallBackend(CeedVectorGetArrayRead(vec, CEED_MEM_DEVICE, &data->points.coords)); | ||
| CeedCallBackend(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &data->points.max_num_points)); | ||
| CeedCallBackend(CeedElemRestrictionGetCompStride(rstr_points, &data->points.coords_comp_stride)); |
There was a problem hiding this comment.
... should this be allowed to change?
There was a problem hiding this comment.
I'm relatively sure that is usually also max num points, see the shared basis functions.
There was a problem hiding this comment.
hmm, if that's the case this a separate duplicate value shouldn't exist
There was a problem hiding this comment.
hmm, if that's the case this a separate duplicate value shouldn't exist
| CeedCallBackend(CeedOperatorAtPointsGetPoints(op, NULL, &vec)); | ||
| CeedCallBackend(CeedOperatorAtPointsGetPoints(op, &rstr_points, &vec)); | ||
| CeedCallBackend(CeedVectorGetArrayRead(vec, CEED_MEM_DEVICE, &data->points.coords)); | ||
| CeedCallBackend(CeedElemRestrictionGetMaxPointsInElement(rstr_points, &data->points.max_num_points)); |
There was a problem hiding this comment.
same here - should only re-compute if the rstr changed (really, should be something we can query if we create an interface to update the points rstr, which we should only allow if the number of elements does not change)
| CeedTransposeMode t_mode, CeedEvalMode eval_mode, CeedVector x_ref, CeedVector u, CeedVector v) { | ||
| Ceed ceed; | ||
| CeedInt Q_1d, dim, max_num_points = num_points[0]; | ||
| CeedInt P_1d, Q_1d, dim, max_num_points = num_points[0]; |
There was a problem hiding this comment.
why did this need to get pulled up?
|
Yeah, overall looks pretty good; I just had a few questions. But we definitely shouldn't merge anything without understanding the performance implications for sure |
2 participants
Purpose:
Reduce the required recompilation of restriction, basis, and operator kernels by changing maximum number of points per element from a compile-time to run-time parameter
Closes: #
LLM/GenAI Disclosure:
None