Query shared memory per SM from the device for RNEA launch configs - #705
Open
maxwbuckley wants to merge 2 commits into
Open
maxwbuckley wants to merge 2 commits into
maxwbuckley wants to merge 2 commits into
Conversation
DynamicsLaunchCfg assumed 100 KB of shared memory per SM when estimating how many blocks can co-reside on an SM. That value is only correct for compute capability 8.6 and 12.x. The occupancy model was therefore wrong on every other supported architecture: 164 KB on 8.0 and 228 KB on 9.0/10.x/11.0, an underestimate of up to 2.3x. Read cudaDevAttrMaxSharedMemoryPerMultiprocessor instead, cached per device since calculate_forward_config and calculate_backward_config run on every kernel launch. When no CUDA device is available the previous 100 KB assumption is kept, so behavior is unchanged on hosts that cannot query the attribute. This only informs which of two already-valid batch counts _warp_align_batches picks; the per-block shared-memory cap and the thread-per-block limit are applied before it runs, so no launch configuration can become invalid. The block shape does change for real robot sizes, which is the point. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
Rationale for the change belongs in the PR, not the source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Max Buckley <maxwbuckley@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DynamicsLaunchCfgassumes a fixed 100 KB of shared memory per SM when estimating how many blocks can co-reside on an SM:That figure is only correct for compute capability 8.6 and 12.x. Per the CUDA Programming Guide, shared memory per SM is 164 KB on 8.0 and 228 KB on 9.0 / 10.x / 11.0, so the occupancy model underestimates resident blocks by up to 2.3x on those parts.
This reads
cudaDevAttrMaxSharedMemoryPerMultiprocessorinstead, cached per device.Why it matters
_warp_align_batchesscores two candidate batch counts byblocks_per_sm × threads, whereblocks_per_sm = sm_capacity // smem_per_block. Because the divisor is fixed, a bigger-than-assumed SM budget changes which candidate wins. It is not a cosmetic constant — the chosen block shape differs for real robot sizes:Safety
The capacity only selects between batch counts that are already valid.
calculate_forward_config/calculate_backward_configapply the per-block shared-memory cap (DEFAULT_MAX_SHARED_MEM) and the 1024 thread-per-block limit before calling_warp_align_batches, and that helper only ever returns the warp-aligned candidate or the next one down — both bounded by its input. So no launch configuration can become invalid, and per-block shared memory can only stay the same or shrink. Tests assert this across all three capacities.When no CUDA device is available the previous 100 KB value is used, so behavior is unchanged on hosts that cannot query the attribute.
Caching
calculate_forward_configruns on everyrnea_forwardlaunch, so the attribute lookup is memoized withlru_cachekeyed on device ordinal. A test asserts the cache is hit rather than re-entering the driver.Testing
curobo/tests/_src/curobolib/test_dynamics_launch_cfg.py, 46 tests:torch.cuda.get_device_properties(...).shared_memory_per_multiprocessorVerified on an RTX 5090 (compute capability 12.0), where the device reports exactly 102400 bytes — so this change is a no-op on 8.6/12.x and only alters behavior on 8.0 / 9.0 / 10.x / 11.0.
I do not have a 9.0/10.x/11.0 part to benchmark end-to-end on, so this is submitted as a correctness fix to the occupancy model rather than a measured speedup. Happy to add numbers if someone can run
benchmark/inverse_dynamics_kernel_benchmark.pyon Hopper/Blackwell.Deliberately out of scope
Two related things I left alone to keep this reviewable — happy to follow up if you're interested:
DEFAULT_MAX_SHARED_MEM = 48 * 1024(also inkinematics_config.py). Raising this requirescudaFuncSetAttribute(cudaFuncAttributeMaxDynamicSharedMemorySize)on the affected kernels — the pattern already exists ingeometry.py::_validate_and_configure_shared_memory— and it trades blocks/SM against batches/block, so it needs measurement rather than a one-line change.blocks_per_smbycudaDevAttrMaxBlocksPerMultiprocessor(16 on 9.0/10.x/11.0/12.x). The current model can compute an unreachable number of resident blocks for small-shared-memory launches.