Skip to content

Optimize value types - #814

Merged
ccummingsNV merged 187 commits into
mainfrom
dev/ccummings/optimize-value-types
Feb 25, 2026
Merged

Optimize value types#814
ccummingsNV merged 187 commits into
mainfrom
dev/ccummings/optimize-value-types

Conversation

@ccummingsNV

@ccummingsNV ccummingsNV commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced tensor creation capabilities through improved bridge APIs with support for multiple scalar types.
    • Added caching mechanism for improved performance in value marshalling operations.
  • Bug Fixes

    • Optimized benchmark tests with tensor reuse and improved gradient handling.
    • Corrected dtype mapping and scalar type code handling.
  • Chores

    • Updated package version to 0.6.0.
    • Refactored internal hook invocation to use centralized bridge abstraction.

@ccummingsNV
ccummingsNV requested a review from a team as a code owner February 25, 2026 10:49
@ccummingsNV
ccummingsNV requested review from bmillsNV and removed request for a team February 25, 2026 10:49
@coderabbitai

coderabbitai Bot commented Feb 25, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The pull request refactors PyTorch tensor operations to use a centralized TorchBridge API instead of direct torch module access. It adds new functions for creating empty tensors, caching device indices, and selecting type-aware writer functions, while removing direct Python hook caching in favor of bridge delegation. The ShaderCursor gains an explicit constructor overload, and value marshalling introduces a caching mechanism for optimized field access patterns.

Changes

Cohort / File(s) Summary
TorchBridge API Extensions
src/slangpy_ext/utils/torch_bridge.h, src/slangpy_ext/utils/torch_bridge.cpp, src/slangpy_ext/utils/torch_bridge.cpp
Adds public APIs create_empty_tensor() and call_torch_autograd_hook() to TorchBridge, includes Python fallback wiring for both, and exposes create_torch_empty_tensor() to Python bindings with shape/scalar_type/device_index parameters.
Tensor Creation Infrastructure
src/slangpy_torch/tensor_bridge_api.h, src/slangpy_torch/torch_bridge_impl.cpp, src/slangpy_torch/pyproject.toml
Defines scalar type code constants and TensorBridge_CreateEmptyTensorFn function pointer in the C API, implements tensor_bridge_create_empty_tensor() in C, bumps API version from 5 to 6, and updates package version to 0.6.0.
PyTorch Tensor Marshalling Refactoring
src/slangpy_ext/utils/slangpytorchtensor.cpp, src/slangpy_ext/utils/slangpytorchtensor.h
Replaces direct torch.empty calls with TorchBridge-based tensor creation, introduces dtype mapping via c10::ScalarType codes, and adds device index caching via mutable m_cached_device_index field.
Value Marshalling Optimization
src/slangpy_ext/utils/slangpyvalue.cpp, src/slangpy_ext/utils/slangpyvalue.h
Introduces private CachedValueWrite struct and ensure_cached() method for lazy initialization of value offset, type layout, and writer; extends pre-dispatch path to use cached writer for optimized field access.
Type-Aware Writer Selection
src/slangpy_ext/device/cursor_utils.h, src/slangpy_ext/device/shader_cursor.cpp
Adds get_writer() method to WriteConverterTable for type-layout-aware writer lookup and exposes get_shader_cursor_writer() public function for external writer retrieval.
ShaderCursor Constructor Extension
src/sgl/device/shader_cursor.h, src/sgl/device/shader_cursor.cpp
Adds new explicit constructor overload ShaderCursor(ShaderObject*, TypeLayoutReflection*, ShaderOffset) for direct initialization without deriving type layout from shader object.
CUDA Device Utilities
src/sgl/device/cuda_utils.h, src/sgl/device/cuda_utils.cpp
Adds get_current_device_index() function to retrieve the active CUDA device index from the current context.
Autograd Hook Centralization
src/slangpy_ext/utils/slangpyfunction.cpp, src/slangpy_ext/utils/slangpyfunction.h, src/slangpy_ext/slangpy_ext.cpp
Removes static s_torch_autograd_hook caching and static_reset() method, delegates autograd hook invocation to TorchBridge, and removes associated cleanup from atexit path.
Python Bridge Fallback
slangpy/torchintegration/bridge_fallback.py
Adds reverse mapping _SCALAR_TYPE_TO_DTYPE and public function create_empty_tensor() with scalar type validation and device-specific CUDA tensor creation.
Test Coverage
slangpy/tests/utils/test_torch_bridge.py
Adds new test class TestTorchBridgeCreateEmptyTensor with parameterized dtype tests, shape/metadata round-trip validation, and writability checks in both native and fallback modes.
Benchmark Configuration
slangpy/benchmarks/test_benchmark_autograd.py
Imports NativeTorchTensorDiffPair, toggles benchmark flags, refactors tensor reuse patterns, and updates backward invocation to use pre-created ones tensor without per-run grad resets.
Build Configuration
.gitignore
Adds ignore patterns for slangpy_torch build artifacts: src/slangpy_torch/\\*.egg-info/, src/slangpy_torch/build/, src/slangpy_torch/dist/.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • bmillsNV
  • tomas-davidovic

Poem

🐰 A bridge was built for tensors to cross,
No more direct torch, no longer lost.
With caching and writers, the marshalling gleams,
Device indices flowing through Bridge's bright streams! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'Optimize value types' aligns with the core changes—introducing efficient caching mechanisms for value marshalling, optimizing tensor creation through TorchBridge APIs, and centralizing type-based writer dispatch logic across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev/ccummings/optimize-value-types

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Nitpick comments (2)
slangpy/benchmarks/test_benchmark_autograd.py (1)

184-184: Remove or replace the dead commented-out code.

# x = x.detach() is now dead code. If detaching is intentionally dropped, remove the line and add a brief comment explaining why it is no longer needed (e.g., why requires_grad=True is safe to pass here).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@slangpy/benchmarks/test_benchmark_autograd.py` at line 184, Remove the dead
commented-out line "# x = x.detach()" from test_benchmark_autograd.py; if
detaching was intentionally removed, replace it with a one-line comment next to
where x is created (referencing the variable x and the surrounding
test/benchmark setup) explaining why detach is no longer needed — e.g., that
passing requires_grad=True is safe here and gradients are not retained between
iterations — so future readers understand the rationale.
src/slangpy_ext/device/cursor_utils.h (1)

409-411: Add Doxygen tags to the new get_writer API comment.

Please include @param type_layout and @return so this declaration matches the project’s C++ documentation standard.

As per coding guidelines, "C++ documentation must use Doxygen format with /// comments and @param/@return tags".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/slangpy_ext/device/cursor_utils.h` around lines 409 - 411, Update the
Doxygen comment for the get_writer function: add an /// `@param` type_layout
description explaining it is the slang::TypeLayoutReflection pointer to resolve
the writer for, and an /// `@return` description stating that the function returns
a std::function<void(CursorType&, nb::object)> which will be empty for types
without a predefined write function; keep the existing /// style and concise
wording to match project C++ documentation standards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/proposals/torch_bridge_fallback.md`:
- Line 11: Replace the outdated install command string "pip install
src/slangpy_torch" in the docs/proposals text with the current package-based
flow by changing it to "pip install slangpy-torch --no-build-isolation"; update
the sentence that references `slangpy_torch` so it instructs users to install
the published package name (`slangpy-torch`) using the `--no-build-isolation`
flag for native mode.

In `@slangpy/benchmarks/test_benchmark_autograd.py`:
- Around line 142-144: The run() closure in test_autograd_slangtorch (using
PolynomialSlangTorch.apply, calling y.backward(ones)) leaves x.grad accumulating
across SUB_ITERATIONS; after each backward() call clear gradients by setting
x.grad to None or zero (e.g., x.grad = None or x.grad.zero_()) to match
test_autograd_pure_torch behavior, and make the same change in the run() closure
for test_autograd_slangpy_manual_hook so x.grad is reset between sub-iterations.
- Around line 184-187: Uncomment the x.detach() call in
PolynomialSlangPyManual.forward so the input tensor x is passed to poly_func
without requires_grad; this prevents detect_torch_tensors from setting
torch_autograd=True and avoids routing through TorchAutoGradHook.apply, ensuring
the manual backward in PolynomialSlangPyManual.backward which calls
poly_func.bwds() remains the sole gradient path.

In `@slangpy/tests/utils/test_torch_bridge.py`:
- Around line 484-493: Replace the mutable list _DTYPE_PARAMS with an immutable
tuple since it is defined at class/module scope and never modified; locate the
_DTYPE_PARAMS declaration and change the enclosing brackets [] to parentheses ()
so the sequence used by pytest.mark.parametrize is immutable while keeping the
same element order and values (SCALAR_UINT8, torch.uint8, etc.).

In `@slangpy/torchintegration/bridge_fallback.py`:
- Line 27: The code currently references torch.complex32 directly which breaks
import on PyTorch <1.12; fix by guarding access to complex32 using
getattr(torch, "complex32", None) and only insert it into the _SCALAR_TYPE_MAP
mapping if not None (e.g., compute _complex32 = getattr(torch, "complex32",
None) and if _complex32: set _SCALAR_TYPE_MAP[_complex32] = 8); update the
existing _SCALAR_TYPE_MAP population logic to remove the direct torch.complex32
literal and perform the conditional update so imports succeed on older PyTorch
versions.

In `@src/slangpy_ext/device/cursor_utils.h`:
- Around line 411-416: In get_writer, guard against a null type_layout before
dereferencing: at the start of get_writer(...) check if type_layout is null and
return an empty std::function (e.g., return {}) to avoid calling
type_layout->getKind() or type_layout->getType() on a null pointer; update the
logic around the existing uses of type_layout->getKind() and
type_layout->getType() in get_writer to assume non-null after the early return.

In `@src/slangpy_ext/utils/slangpytorchtensor.cpp`:
- Around line 757-759: The code currently calls
TorchBridge::instance().create_empty_tensor(..., 0) which hardcodes CUDA device
0; change this to determine the correct device from the call context (e.g.,
infer from input tensors or current CUDA stream) and pass that device index to
create_empty_tensor instead of 0. Locate the call that builds shape_vec and
replace the literal 0 with a device id obtained via the appropriate helper (for
example a function that returns the executing device or by reading the device
from an input tensor), or add a device parameter to the surrounding function so
create_empty_tensor receives the correct device for the current execution
context.

In `@src/slangpy_ext/utils/slangpyvalue.cpp`:
- Around line 15-24: The current NativeValueMarshall::ensure_cached uses only
m_cached.is_valid so the cached value_offset/value_type_layout/writer can be
reused for different bindings; fix by keying or validating the cache on each
call: add a cache key (e.g. binding pointer or binding->variable_name()) stored
alongside m_cached and, in ensure_cached, compare the incoming binding identity
(or resolve ShaderCursor field = cursor[binding->variable_name()]["value"] and
compare field.offset() and field.slang_type_layout() against
m_cached.value_offset and m_cached.value_type_layout) and only reuse cached
writer if they match, otherwise refresh m_cached.value_offset,
m_cached.value_type_layout and m_cached.writer via get_shader_cursor_writer;
mirror the same approach for NativeTensorMarshall to restore/replace the
disabled validation logic instead of a single is_valid flag.

In `@src/slangpy_ext/utils/slangpyvalue.h`:
- Around line 40-43: NativeValueMarshall::ensure_cached currently returns early
if m_cached is populated but doesn't re-validate that cached.value_offset,
cached.value_type_layout and cached.writer still match the provided ShaderCursor
and NativeBoundVariableRuntime; implement the same re-validation pattern used by
NativeTensorMarshall: re-resolve the current field/offset/type_layout/writer
from the given ShaderCursor and binding and compare those values to m_cached; if
any differ, recompute and replace m_cached (or invalidate it) before returning.
Ensure you use the existing mutable CachedValueWrite m_cached so the const
ensure_cached can update it, and add an invalidation hook or comparison check
when binding/layout changes to prevent reuse of stale offsets/writers.

In `@src/slangpy_torch/torch_bridge_impl.cpp`:
- Around line 246-252: The code performs pointer arithmetic with `shape + ndim`
without ensuring `ndim` is non-negative; add a defensive check before using
`shape` and before the `std::vector<int64_t> shape_vec(shape, shape + ndim)`
construction (e.g., if `ndim < 0` return nullptr or handle as an error), and
also ensure the existing `if (!shape && ndim > 0) return nullptr;` logic is
updated/ordered so the `ndim < 0` case is handled first to avoid undefined
behavior when building `shape_vec`.

---

Nitpick comments:
In `@slangpy/benchmarks/test_benchmark_autograd.py`:
- Line 184: Remove the dead commented-out line "# x = x.detach()" from
test_benchmark_autograd.py; if detaching was intentionally removed, replace it
with a one-line comment next to where x is created (referencing the variable x
and the surrounding test/benchmark setup) explaining why detach is no longer
needed — e.g., that passing requires_grad=True is safe here and gradients are
not retained between iterations — so future readers understand the rationale.

In `@src/slangpy_ext/device/cursor_utils.h`:
- Around line 409-411: Update the Doxygen comment for the get_writer function:
add an /// `@param` type_layout description explaining it is the
slang::TypeLayoutReflection pointer to resolve the writer for, and an ///
`@return` description stating that the function returns a
std::function<void(CursorType&, nb::object)> which will be empty for types
without a predefined write function; keep the existing /// style and concise
wording to match project C++ documentation standards.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1f686b and 02d8bd1.

📒 Files selected for processing (20)
  • .gitignore
  • docs/proposals/torch_bridge_fallback.md
  • slangpy/benchmarks/test_benchmark_autograd.py
  • slangpy/tests/utils/test_torch_bridge.py
  • slangpy/torchintegration/bridge_fallback.py
  • src/sgl/device/shader_cursor.cpp
  • src/sgl/device/shader_cursor.h
  • src/slangpy_ext/device/cursor_utils.h
  • src/slangpy_ext/device/shader_cursor.cpp
  • src/slangpy_ext/slangpy_ext.cpp
  • src/slangpy_ext/utils/slangpyfunction.cpp
  • src/slangpy_ext/utils/slangpyfunction.h
  • src/slangpy_ext/utils/slangpytorchtensor.cpp
  • src/slangpy_ext/utils/slangpyvalue.cpp
  • src/slangpy_ext/utils/slangpyvalue.h
  • src/slangpy_ext/utils/torch_bridge.cpp
  • src/slangpy_ext/utils/torch_bridge.h
  • src/slangpy_torch/pyproject.toml
  • src/slangpy_torch/tensor_bridge_api.h
  • src/slangpy_torch/torch_bridge_impl.cpp
💤 Files with no reviewable changes (2)
  • src/slangpy_ext/utils/slangpyfunction.h
  • src/slangpy_ext/slangpy_ext.cpp

Comment thread docs/proposals/torch_bridge_fallback.md Outdated
Comment thread slangpy/benchmarks/test_benchmark_autograd.py
Comment thread slangpy/benchmarks/test_benchmark_autograd.py Outdated
Comment thread slangpy/tests/utils/test_torch_bridge.py
Comment thread slangpy/torchintegration/bridge_fallback.py Outdated
Comment thread src/slangpy_ext/device/cursor_utils.h
Comment thread src/slangpy_ext/utils/slangpytorchtensor.cpp
Comment thread src/slangpy_ext/utils/slangpyvalue.cpp
Comment thread src/slangpy_ext/utils/slangpyvalue.h
Comment thread src/slangpy_torch/torch_bridge_impl.cpp

@tdavidovicNV tdavidovicNV left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Makes sense, the only thing that raised a red flag is the swap of which benchmarks are True and False.

Comment thread slangpy/benchmarks/test_benchmark_autograd.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

♻️ Duplicate comments (4)
src/slangpy_ext/device/cursor_utils.h (1)

409-429: The previous null-guard concern is now resolved.

The if (!type_layout) return {}; guard at Line 413 directly addresses the past review comment.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/slangpy_ext/device/cursor_utils.h` around lines 409 - 429, The get_writer
function's previous null-pointer issue has been addressed by adding a guard at
the start; keep the early-return check "if (!type_layout) return {};" in
get_writer and ensure the subsequent calls use the validated pointer
(type_layout->getKind(), type_layout->getType()) as implemented in get_writer to
avoid dereferencing null; no further changes required to
m_write_scalar/m_write_vector/m_write_matrix resolution logic.
slangpy/benchmarks/test_benchmark_autograd.py (1)

142-145: ⚠️ Potential issue | 🟡 Minor

Clear gradients between sub-iterations to avoid accumulation skew.

Line 144 and Line 210 call backward() repeatedly on the same x without resetting x.grad, unlike the pure torch path (Line 79). This can skew later sub-iterations.

Proposed fix
     def run() -> None:
         y = PolynomialSlangTorch.apply(a_val, b_val, c_val, x)
         y.backward(ones)  # type: ignore[union-attr]
+        if x.grad is not None:
+            x.grad.zero_()
     def run() -> None:
         y = PolynomialSlangPyManual.apply(a_val, b_val, c_val, x)
         y.backward(ones)  # type: ignore[union-attr]
+        if x.grad is not None:
+            x.grad.zero_()
#!/bin/bash
set -euo pipefail

echo "=== pure torch run() ==="
sed -n '74,82p' slangpy/benchmarks/test_benchmark_autograd.py

echo
echo "=== slangtorch run() ==="
sed -n '142,148p' slangpy/benchmarks/test_benchmark_autograd.py

echo
echo "=== manual hook run() ==="
sed -n '208,214p' slangpy/benchmarks/test_benchmark_autograd.py

Expected result: only the pure torch run currently resets gradients; the other two do not.

Also applies to: 208-210

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@slangpy/benchmarks/test_benchmark_autograd.py` around lines 142 - 145, The
slangtorch and manual-hook benchmark run functions call backward repeatedly on
the same tensor (PolynomialSlangTorch.apply(...) and the manual-hook run)
without clearing x.grad, causing accumulation skew; update both run functions to
mimic the pure-torch path by zeroing or resetting x.grad (e.g., if x.grad is not
None: x.grad.zero_() or x.grad = None) before calling y.backward(ones) so each
sub-iteration starts with no accumulated gradients.
src/slangpy_ext/utils/slangpytorchtensor.cpp (1)

757-763: ⚠️ Potential issue | 🟠 Major

Avoid sticky device-index caching across calls.

Caching m_cached_device_index once can allocate outputs on the wrong GPU after device/context changes. This keeps the same root risk as the earlier hardcoded-device issue.

Proposed fix
-    if (m_cached_device_index < 0)
-        m_cached_device_index = static_cast<int32_t>(cuda::get_current_device_index());
-    int32_t device_index = m_cached_device_index;
+    // Re-resolve each call to stay correct under context/device switches.
+    int32_t device_index = static_cast<int32_t>(cuda::get_current_device_index());
+    m_cached_device_index = device_index;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/slangpy_ext/utils/slangpytorchtensor.cpp` around lines 757 - 763,
m_cached_device_index is causing outputs to be allocated on a stale GPU; stop
reusing it across calls by querying the current device at allocation time
instead of caching: replace the m_cached_device_index lookup in the function
with a direct call to cuda::get_current_device_index() (or ensure
m_cached_device_index is invalidated on context changes) and pass that fresh
device_index into TorchBridge::instance().create_empty_tensor(...); reference
m_cached_device_index, cuda::get_current_device_index(), and
TorchBridge::instance().create_empty_tensor to locate the change.
slangpy/torchintegration/bridge_fallback.py (1)

32-34: LGTM — complex32 guard correctly addresses the previous concern.

The getattr guard prevents import failure on PyTorch < 1.12, and the conditional insertion into _SCALAR_TYPE_MAP is correct.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@slangpy/torchintegration/bridge_fallback.py` around lines 32 - 34, The guard
using getattr for torch.complex32 and the conditional insertion into
_SCALAR_TYPE_MAP are correct; no code changes needed — keep the _complex32 =
getattr(torch, "complex32", None) check and the subsequent if _complex32 is not
None: _SCALAR_TYPE_MAP[_complex32] = 8  # TENSOR_BRIDGE_SCALAR_COMPLEX32 as-is.
🧹 Nitpick comments (2)
src/slangpy_ext/device/cursor_utils.h (1)

409-411: Missing @param and @return Doxygen tags.

The doc comment uses plain /// prose but omits @param and @return tags required by the project's Doxygen convention.

📝 Proposed fix
-    /// Resolve a type-specialized writer function for the given type layout.
-    /// Returns an empty function for types that do not have a predefined write function.
+    /// Resolve a type-specialized writer function for the given type layout.
+    /// `@param` type_layout The Slang type layout to look up. May be null.
+    /// `@return` The corresponding writer, or an empty function if the type is unsupported or null.
     std::function<void(CursorType&, nb::object)> get_writer(slang::TypeLayoutReflection* type_layout) const

As per coding guidelines: "C++ documentation must use Doxygen format with /// comments and @param/@return tags."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/slangpy_ext/device/cursor_utils.h` around lines 409 - 411, The Doxygen
comment for get_writer(CursorType&, nb::object) is missing required `@param` and
`@return` tags; update the comment above std::function<void(CursorType&,
nb::object)> get_writer(slang::TypeLayoutReflection* type_layout) const to use
Doxygen format: add an `@param` describing the type_layout parameter (e.g., "type
layout to resolve a specialized writer for"), add an `@return` describing the
returned std::function (e.g., "a writer that writes the given type into a
CursorType or an empty function if none exists"), and ensure CursorType and
slang::TypeLayoutReflection are referenced in the descriptions for clarity.
slangpy/benchmarks/test_benchmark_autograd.py (1)

115-117: Use the helper-derived device instead of hardcoded "cuda".

Line 115 and Line 176 hardcode "cuda" even though helpers.get_torch_device(device_type) is already computed. Using the helper device keeps parametrized benchmarks portable.

Proposed fix
-    x = torch.randn(n, dtype=torch.float32, device="cuda", requires_grad=True)
+    x = torch.randn(n, dtype=torch.float32, device=device, requires_grad=True)
     ones = torch.ones_like(x)
-    x = torch.randn(n, dtype=torch.float32, device="cuda", requires_grad=True)
+    x = torch.randn(n, dtype=torch.float32, device=device, requires_grad=True)
     ones = torch.ones_like(x)

As per coding guidelines, "Use helpers.get_device(device_type) or helpers.get_torch_device(device_type) for device creation in benchmark tests".

Also applies to: 176-178

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@slangpy/benchmarks/test_benchmark_autograd.py` around lines 115 - 117,
Replace hardcoded "cuda" device strings with the helper-derived torch device:
call helpers.get_torch_device(device_type) (or reuse the existing torch_device
variable if present) and pass that into tensor creation instead of "cuda" (e.g.,
use torch.randn(n, dtype=torch.float32, device=torch_device, requires_grad=True)
and ensure the other occurrence that uses "cuda" at the later block also uses
torch_device). This ensures both the x and the later tensors use the
helper-provided device.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@slangpy/benchmarks/test_benchmark_autograd.py`:
- Line 53: The test currently enables a heavy manual benchmark by default via
the RUN_SLANGPY_MANUAL_HOOK_BENCHMARK flag; change its default value to False
(set RUN_SLANGPY_MANUAL_HOOK_BENCHMARK = False) so normal test runs don’t
execute the expensive ITERATIONS * SUB_ITERATIONS path, and add a short inline
comment indicating it should be enabled explicitly (or via an environment
variable) only when profiling; keep the flag name
RUN_SLANGPY_MANUAL_HOOK_BENCHMARK so callers/tests can override it when needed.

In `@slangpy/torchintegration/bridge_fallback.py`:
- Around line 216-229: Update create_empty_tensor to use a precise shape
annotation (List[int] or list[int]) and add a CUDA availability guard: check
torch.cuda.is_available() at the start of the function and raise a ValueError
with a clear message if CUDA is unavailable; keep the existing scalar-type
lookup via _SCALAR_TYPE_TO_DTYPE and the ValueError for unsupported scalar_type,
then call torch.empty(..., device=f"cuda:{device_index}") only after the CUDA
check passes.
- Around line 216-229: Add unit tests for create_empty_tensor in
slangpy/tests/utils/test_torch_bridge.py that run under both bridge modes using
the torch_bridge_mode fixture; write parametrized tests that call
create_empty_tensor with multiple shapes (e.g., scalar, 1D, 2D), several
scalar_type codes mapped in _SCALAR_TYPE_TO_DTYPE (e.g., float32, int64), and at
least two CUDA device_index values (or skip if CUDA unavailable), then assert
the returned torch.Tensor has the expected shape, dtype, device and is
contiguous; ensure tests run for both native (torch_bridge_impl) and fallback
(bridge_fallback.create_empty_tensor) modes via the fixture and include
appropriate skips/markers for missing CUDA.

In `@src/sgl/device/cuda_utils.cpp`:
- Around line 17-25: get_current_device_index currently errors out when no CUDA
context is active; change it to return 0 in that case. Specifically, after
calling cuCtxGetCurrent(&cu_context) in get_current_device_index, if cu_context
is nullptr (or cuCtxGetCurrent indicates no context) do not call SGL_CHECK or
cuCtxGetDevice — instead return 0 immediately; otherwise proceed to call
cuCtxGetDevice and keep the existing checks (SGL_CU_CHECK and SGL_CHECK) for the
valid-context path.

In `@src/sgl/device/cuda_utils.h`:
- Around line 30-32: Update the Doxygen comment for get_current_device_index()
to include an explicit `@return` tag describing the returned value; keep the
existing /// comment style and mention that it returns the CUDA device index for
the current CUDA context or 0 if no CUDA context is active, so tools and readers
get the required return documentation for the SGL_API int
get_current_device_index() API.

In `@src/slangpy_ext/device/cursor_utils.h`:
- Around line 422-425: The vector/matrix branches in get_writer risk
out-of-bounds access because getColumnCount()/getRowCount() are used directly to
index m_write_vector/m_write_matrix; update the TypeReflection::Kind::vector and
::matrix cases in get_writer to validate the scalar type and that column/row
counts are within the bounds (0 < count < 5 or whatever the declared dimension
size is) before indexing m_write_vector and m_write_matrix, and return an
empty/default result when the counts are out of range or the scalar type is
invalid so callers never crash on malformed or unexpected Slang reflection
results.

---

Duplicate comments:
In `@slangpy/benchmarks/test_benchmark_autograd.py`:
- Around line 142-145: The slangtorch and manual-hook benchmark run functions
call backward repeatedly on the same tensor (PolynomialSlangTorch.apply(...) and
the manual-hook run) without clearing x.grad, causing accumulation skew; update
both run functions to mimic the pure-torch path by zeroing or resetting x.grad
(e.g., if x.grad is not None: x.grad.zero_() or x.grad = None) before calling
y.backward(ones) so each sub-iteration starts with no accumulated gradients.

In `@slangpy/torchintegration/bridge_fallback.py`:
- Around line 32-34: The guard using getattr for torch.complex32 and the
conditional insertion into _SCALAR_TYPE_MAP are correct; no code changes needed
— keep the _complex32 = getattr(torch, "complex32", None) check and the
subsequent if _complex32 is not None: _SCALAR_TYPE_MAP[_complex32] = 8  #
TENSOR_BRIDGE_SCALAR_COMPLEX32 as-is.

In `@src/slangpy_ext/device/cursor_utils.h`:
- Around line 409-429: The get_writer function's previous null-pointer issue has
been addressed by adding a guard at the start; keep the early-return check "if
(!type_layout) return {};" in get_writer and ensure the subsequent calls use the
validated pointer (type_layout->getKind(), type_layout->getType()) as
implemented in get_writer to avoid dereferencing null; no further changes
required to m_write_scalar/m_write_vector/m_write_matrix resolution logic.

In `@src/slangpy_ext/utils/slangpytorchtensor.cpp`:
- Around line 757-763: m_cached_device_index is causing outputs to be allocated
on a stale GPU; stop reusing it across calls by querying the current device at
allocation time instead of caching: replace the m_cached_device_index lookup in
the function with a direct call to cuda::get_current_device_index() (or ensure
m_cached_device_index is invalidated on context changes) and pass that fresh
device_index into TorchBridge::instance().create_empty_tensor(...); reference
m_cached_device_index, cuda::get_current_device_index(), and
TorchBridge::instance().create_empty_tensor to locate the change.

---

Nitpick comments:
In `@slangpy/benchmarks/test_benchmark_autograd.py`:
- Around line 115-117: Replace hardcoded "cuda" device strings with the
helper-derived torch device: call helpers.get_torch_device(device_type) (or
reuse the existing torch_device variable if present) and pass that into tensor
creation instead of "cuda" (e.g., use torch.randn(n, dtype=torch.float32,
device=torch_device, requires_grad=True) and ensure the other occurrence that
uses "cuda" at the later block also uses torch_device). This ensures both the x
and the later tensors use the helper-provided device.

In `@src/slangpy_ext/device/cursor_utils.h`:
- Around line 409-411: The Doxygen comment for get_writer(CursorType&,
nb::object) is missing required `@param` and `@return` tags; update the comment
above std::function<void(CursorType&, nb::object)>
get_writer(slang::TypeLayoutReflection* type_layout) const to use Doxygen
format: add an `@param` describing the type_layout parameter (e.g., "type layout
to resolve a specialized writer for"), add an `@return` describing the returned
std::function (e.g., "a writer that writes the given type into a CursorType or
an empty function if none exists"), and ensure CursorType and
slang::TypeLayoutReflection are referenced in the descriptions for clarity.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 02d8bd1 and 3e31878.

📒 Files selected for processing (7)
  • slangpy/benchmarks/test_benchmark_autograd.py
  • slangpy/torchintegration/bridge_fallback.py
  • src/sgl/device/cuda_utils.cpp
  • src/sgl/device/cuda_utils.h
  • src/slangpy_ext/device/cursor_utils.h
  • src/slangpy_ext/utils/slangpytorchtensor.cpp
  • src/slangpy_ext/utils/slangpytorchtensor.h

Comment thread slangpy/benchmarks/test_benchmark_autograd.py
Comment thread slangpy/torchintegration/bridge_fallback.py
Comment thread src/sgl/device/cuda_utils.cpp
Comment thread src/sgl/device/cuda_utils.h
Comment thread src/slangpy_ext/device/cursor_utils.h
@ccummingsNV
ccummingsNV merged commit 5278620 into main Feb 25, 2026
23 checks passed
@ccummingsNV
ccummingsNV deleted the dev/ccummings/optimize-value-types branch February 25, 2026 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants