Cleanup shader cursor

slangpy/tests/device/test_buffer_cursor.py

@@ -242,13 +242,8 @@ def get_this(self) -> "SelfReturningWrapper":
 ]
 
 
-# Filter out all bool tests for CUDA/Metal backend, as it is not handled correct. See issue:
-# https://github.com/shader-slang/slangpy/issues/274
 def get_tests(device_type: spy.DeviceType):
-    if device_type not in [spy.DeviceType.cuda, spy.DeviceType.metal]:
-        return TESTS
-    tests = [x for x in TESTS if "bool1" not in x[0]]
-    return tests
+    return TESTS
 
 
 def variable_decls(tests: list[Any]):

slangpy/tests/device/test_shader_cursor.py

@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
+import gc
 import pytest
 import sys
 import struct
@@ -63,6 +64,8 @@ class Var:
     # bool
     "u_bool_false": Var(kind="scalar", type="bool", value=False),
     "u_bool_true": Var(kind="scalar", type="bool", value=True),
+    # bool1
+    "u_bool1": Var(kind="vector", type="bool", value=[True]),
     # bool2
     "u_bool2": Var(kind="vector", type="bool", value=[False, True]),
     # bool3
@@ -339,6 +342,21 @@ def write_vars(
     with command_encoder.begin_compute_pass() as pass_encoder:
         shader_object = pass_encoder.bind_pipeline(kernel.pipeline)
         cursor = spy.ShaderCursor(shader_object)
+
+        assert not cursor.find_element(0).is_valid()
+        assert not cursor.find_entry_point(1).is_valid()
+        assert not cursor.get_field_by_index(1_000_000).is_valid()
+        assert not cursor["u_int_array"].find_element(4).is_valid()
+        assert not cursor["u_int4"].find_element(4).is_valid()
+        assert not cursor["u_float2x2"].find_element(2).is_valid()
+        with pytest.raises(IndexError):
+            cursor["u_int_array"][4]
+
+        # ShaderCursor stores a native pointer to its ShaderObject. The Python binding
+        # must keep that owner alive even when its original Python variable is dropped.
+        del shader_object
+        gc.collect()
+
         cursor["results"] = result_buffer
         write_vars(device_type, cursor, TEST_VARS)
         pass_encoder.dispatch(thread_count=[1, 1, 1])
@@ -356,12 +374,6 @@ def write_vars(
     for named_typed_result, named_typed_reference in zip(
         named_typed_results, named_typed_references
     ):
-        # Vulkan/Metal/CUDA packing rule for certain matrix types are not the same as D3D12's
-        if (device_type in [spy.DeviceType.vulkan, spy.DeviceType.metal, spy.DeviceType.cuda]) and (
-            named_typed_result[0] == "u_float2x2" or named_typed_result[0] == "u_float3x3"
-        ):
-            continue
-
         assert named_typed_result == named_typed_reference
 
 

slangpy/tests/device/test_shader_cursor.slang

@@ -28,6 +28,7 @@ struct Test {
 uniform bool u_bool_false;
 uniform bool u_bool_true;
 
+uniform bool1 u_bool1;
 uniform bool2 u_bool2;
 uniform bool3 u_bool3;
 uniform bool4 u_bool4;
@@ -203,6 +204,7 @@ void compute_main(uint3 tid: SV_DispatchThreadID)
     writer.write(u_bool_false);
     writer.write(u_bool_true);
 
+    writer.write(u_bool1);
     writer.write(u_bool2);
     writer.write(u_bool3);
     writer.write(u_bool4);

src/sgl/device/buffer_cursor.cpp

@@ -186,9 +186,7 @@ template void CursorWriteWrappers<BufferElementCursor, size_t>::_set_vector(
     }
 
 GETSET_SCALAR(bool, bool_);
-// bool1 case specifically cannot be handled due to:
-// https://github.com/shader-slang/slang/issues/7441
-// GETSET_VECTOR(bool1, bool_);
+GETSET_VECTOR(bool1, bool_);
 GETSET_VECTOR(bool2, bool_);
 GETSET_VECTOR(bool3, bool_);
 GETSET_VECTOR(bool4, bool_);
@@ -251,23 +249,6 @@ GETSET_SCALAR(double, float64);
 #undef GETSET_VECTOR
 #undef GETSET_MATRIX
 
-// Template specialization to allow setting booleans on a parameter block.
-// On the host side a bool is 1B and the device 4B. We cast bools to 32-bit integers here.
-// Note that this applies to our boolN vectors as well, which are currently 1B per element.
-
-template<>
-SGL_API void BufferElementCursor::set(const bool1& v) const
-{
-    SGL_CHECK(_get_device_type() != DeviceType::cuda, "bool1 currently not supported due to CUDA backend issues.");
-    _set_vector(&v, sizeof(v), TypeReflection::ScalarType::bool_, 1);
-}
-template<>
-SGL_API void BufferElementCursor::get(bool1& v) const
-{
-    SGL_CHECK(_get_device_type() != DeviceType::cuda, "bool1 currently not supported due to CUDA backend issues.");
-    _get_vector(&v, sizeof(v), TypeReflection::ScalarType::bool_, 1);
-}
-
 template<>
 SGL_API void BufferElementCursor::get(DescriptorHandle& value) const
 {

src/sgl/device/cursor_access_wrappers.h

@@ -21,22 +21,12 @@ class SGL_API CursorWriteWrappers {
 
     // The array/vector of elements has two special cases where things are not tightly packed
     //
-    // First one is bool, with the following options:
-    //                cpu_element_size | element_stride | element_size
-    // HLSL                          1 |              4 |            4
-    // CUDA - array                  1 |              1 |            1
-    // CUDA - vector (old)           1 |              4 |            1
-    // CUDA - vector (new)           1 |              1 |            1
+    // First one is bool. HLSL uniform layouts use four-byte bool elements, while
+    // CUDA layouts use one-byte elements.
     //
     // When element_size != cpu_element_size, we need to convert between bool and uint32_t.
     // This is necessary to make sure we do not accidentally ignore bits 8-31 in either read or write.
     //
-    // Further caveat is that CUDA - vector (old) says that the element_size == 1, but is actually implemented using int
-    // in the backend, which is then cast to bool. But if we use that knowledge (to avoid ignoring bits 8-31),
-    // we break future compatibility. So for CUDA vector (old), we are ignoring bits 8-31 in the boolX implementations,
-    // and as such CUDA code that reports "true" because the stored value is 256, will report "false" on the CPU.
-    //
-    //
     // The other case is float4x3, where in HLSL we have a row-stride of 16B in some cases.
 
     void _set_array_or_vector(
@@ -49,17 +39,6 @@ class SGL_API CursorWriteWrappers {
         // CPU is assumed tightly packed, i.e., stride and size are the same value.
         size_t cpu_element_size = cursor_utils::get_scalar_type_cpu_size(cpu_scalar_type);
         size_t element_stride = _get_slang_type_layout()->getElementStride(SLANG_PARAMETER_CATEGORY_UNIFORM);
-        // CUDA misreports the actual element stride, see https://github.com/shader-slang/slang/issues/7441
-        // In the old implementation, bool2-4 are implemented as int2-4, and even though bool1 is implemented as int1,
-        // the actual emitted code is bool. So for bool2-4, the element stride is 4, for bool1 it remains at 1.
-        // The check for the total size == sizeof(int) * element_count is to disable this on newer Slang implementation,
-        // where bool1-4 is implemented as an actual struct of 1-4 bools.
-        if (cpu_scalar_type == TypeReflection::ScalarType::bool_ && _get_device_type_internal() == DeviceType::cuda
-            && _get_slang_type_layout()->getKind() == slang::TypeReflection::Kind::Vector
-            && _get_slang_type_layout()->getSize() == sizeof(int) * element_count) {
-            if (element_count > 1)
-                element_stride = 4;
-        }
         size_t element_size = _get_slang_type_layout()->getElementTypeLayout()->getSize();
 
         SGL_CHECK(
@@ -125,6 +104,7 @@ class SGL_API CursorWriteWrappers {
     void
     _set_array(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, size_t element_count) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_array(_get_slang_type_layout(), size, cpu_scalar_type, element_count);
 #endif
@@ -133,6 +113,7 @@ class SGL_API CursorWriteWrappers {
 
     void _set_scalar(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_scalar(_get_slang_type_layout(), size, cpu_scalar_type);
 #else
@@ -154,6 +135,7 @@ class SGL_API CursorWriteWrappers {
 
     void _set_vector(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int dimension) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_vector(_get_slang_type_layout(), size, cpu_scalar_type, dimension);
 #endif
@@ -164,6 +146,7 @@ class SGL_API CursorWriteWrappers {
     void
     _set_matrix(const void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int rows, int cols) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
         // matrix has element type (rows) which has element type (individual cells).
         // we are currently shortcuiting that logic only handling the case where float3x3 is in memory
         // represented as float3x4.
@@ -216,7 +199,7 @@ class SGL_API CursorWriteWrappers {
         return static_cast<const BaseCursor*>(this)->slang_type_layout();
     }
 
-    DeviceType _get_device_type_internal() const { return static_cast<const BaseCursor*>(this)->_get_device_type(); }
+    bool _is_valid_internal() const { return static_cast<const BaseCursor*>(this)->is_valid(); }
 };
 
 template<typename BaseCursor, typename TOffset>
@@ -233,17 +216,6 @@ class SGL_API CursorReadWrappers {
         // CPU is assumed tightly packed, i.e., stride and size are the same value.
         size_t cpu_element_size = cursor_utils::get_scalar_type_cpu_size(cpu_scalar_type);
         size_t element_stride = _get_slang_type_layout()->getElementStride(SLANG_PARAMETER_CATEGORY_UNIFORM);
-        // CUDA misreports the actual element stride, see https://github.com/shader-slang/slang/issues/7441
-        // In the old implementation, bool2-4 are implemented as int2-4, and even though bool1 is implemented as int1,
-        // the actual emitted code is bool. So for bool2-4, the element stride is 4, for bool1 it remains at 1.
-        // The check for the total size == sizeof(int) * element_count is to disable this on newer Slang implementation,
-        // where bool1-4 is implemented as an actual struct of 1-4 bools.
-        if (cpu_scalar_type == TypeReflection::ScalarType::bool_ && _get_device_type_internal() == DeviceType::cuda
-            && _get_slang_type_layout()->getKind() == slang::TypeReflection::Kind::Vector
-            && _get_slang_type_layout()->getSize() == sizeof(int) * element_count) {
-            if (element_count > 1)
-                element_stride = 4;
-        }
         size_t element_size = _get_slang_type_layout()->getElementTypeLayout()->getSize();
 
         SGL_CHECK(
@@ -304,6 +276,7 @@ class SGL_API CursorReadWrappers {
 public:
     void _get_array(void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, size_t element_count) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_array(_get_slang_type_layout(), size, cpu_scalar_type, element_count);
 #endif
@@ -312,6 +285,7 @@ class SGL_API CursorReadWrappers {
 
     void _get_scalar(void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_scalar(_get_slang_type_layout(), size, cpu_scalar_type);
 #endif
@@ -333,6 +307,7 @@ class SGL_API CursorReadWrappers {
 
     void _get_vector(void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int dimension) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
 #ifdef SGL_ENABLE_CURSOR_TYPE_CHECKS
         cursor_utils::check_vector(_get_slang_type_layout(), size, cpu_scalar_type, dimension);
 #else
@@ -344,6 +319,7 @@ class SGL_API CursorReadWrappers {
 
     void _get_matrix(void* data, size_t size, TypeReflection::ScalarType cpu_scalar_type, int rows, int cols) const
     {
+        SGL_CHECK(_is_valid_internal(), "Invalid cursor");
         // matrix has element type (rows) which has element type (individual cells).
         // we are currently shortcuiting that logic only handling the case where float3x3 is in memory
         // represented as float3x4.
@@ -397,7 +373,7 @@ class SGL_API CursorReadWrappers {
         return static_cast<const BaseCursor*>(this)->slang_type_layout();
     }
 
-    DeviceType _get_device_type_internal() const { return static_cast<const BaseCursor*>(this)->_get_device_type(); }
+    bool _is_valid_internal() const { return static_cast<const BaseCursor*>(this)->is_valid(); }
 };