Skip to content

Fix lowering of swizzled lvalues #12769

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jkwak-work merged 5 commits into from
Sep 1, 2026
+36 −4
Merged

Fix lowering of swizzled lvalues #12769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b84affd
Fix lowering of swizzled lvalues
kmshanah Aug 26, 2026
ea164f6
Fix formatting
kmshanah Aug 27, 2026
6ae1580
Revert backpermute comments to previous version, just with updated pa…
kmshanah Aug 28, 2026
93438cc
Remove redundant .expected.txt file
kmshanah Aug 29, 2026
89d1da1
Add missing filecheck-buffer annotation to vector swizzle test
kmshanah Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions source/slang/slang-lower-to-ir.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7799,12 +7799,14 @@ struct LValueExprLoweringVisitor : ExprLoweringVisitorBase<LValueExprLoweringVis
auto loweredBase = lowerLValueExpr(context, expr->base);
UInt elementCount = (UInt)expr->elementIndices.getCount();

// Assign to 'bs' the elements from 'as' according to the first 'n' indices in 'is'
auto backpermute = [](UInt n, const auto as, const auto is, auto bs)
// Assign to `resultElements` the elements from `sourceElements` according to the first `n`
// indices in `indices`
auto backpermute =
[](UInt n, const auto& sourceElements, const auto& indices, auto& resultElements)
Comment thread
jkwak-work marked this conversation as resolved.
{
for (UInt i = 0; i < n; ++i)
{
bs[i] = as[is[i]];
resultElements[i] = sourceElements[indices[i]];
}
};

Expand All @@ -7829,7 +7831,10 @@ struct LValueExprLoweringVisitor : ExprLoweringVisitorBase<LValueExprLoweringVis
RefPtr<SwizzledLValueInfo> swizzledLValue = new SwizzledLValueInfo;
swizzledLValue->type = irType;
swizzledLValue->base = baseSwizzleInfo->base;
swizzledLValue->elementIndices.add((uint32_t)elementCount);

// Set the count of indices and leave them uninitialized.
// This is safe because `backpermute` fills all `elementCount` slots below.
swizzledLValue->elementIndices.setCount((uint32_t)elementCount);
Comment thread
jkwak-work marked this conversation as resolved.
Comment thread
jkwak-work marked this conversation as resolved.
Comment thread
jkwak-work marked this conversation as resolved.
Comment thread
jkwak-work marked this conversation as resolved.
Comment thread
jkwak-work marked this conversation as resolved.
Comment thread
jkwak-work marked this conversation as resolved.

@github-actions github-actions Bot Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Clarity: make the "every slot is written" invariant local

The safety comment reasons in terms of elementCount, but the backpermute call below takes its loop bound from swizzledLValue->elementIndices.getCount(), not elementCount:

backpermute(
    swizzledLValue->elementIndices.getCount(),
    ...
    swizzledLValue->elementIndices);

The two are equal only because setCount(elementCount) was just called, so "backpermute fills all elementCount slots" is only obvious once the reader connects setCount here to the later getCount(). Passing elementCount directly to backpermute (or SLANG_ASSERT(swizzledLValue->elementIndices.getCount() == elementCount)) would make the write-every-slot invariant self-evident at this line.

Separately, "leave them uninitialized" is only accurate for the POD short-buffer case (setCount default-constructs elements in the overflow-buffer path). Since the load-bearing fact is that backpermute overwrites every slot before any read, consider leading with that and dropping the incidental initialization detail.

Comment thread
jkwak-work marked this conversation as resolved.

// Take the swizzle element of the "outer" swizzle, as it was
// written by the user. In our running example of `foo[i].zw.y`
Expand Down
27 changes: 27 additions & 0 deletions tests/language-feature/swizzles/vector-swizzle-write-swizzle.slang
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=CHECK): -compute -shaderobj -output-using-type
//TEST(compute, vulkan):COMPARE_COMPUTE(filecheck-buffer=CHECK): -vk -compute -shaderobj -output-using-type
//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=CHECK):-slang -shaderobj -mtl -output-using-type
//TEST(compute):COMPARE_COMPUTE(filecheck-buffer=CHECK): -cpu -compute -shaderobj -output-using-type

// Test that writing to swizzles of vector swizzles works correctly

// CHECK: 2.0
// CHECK-NEXT: 1.0
// CHECK-NEXT: 0.0
// CHECK-NEXT: 9.0

//TEST_INPUT:ubuffer(data=[0 0 0 0], stride=4):out,name=outputBuffer
RWStructuredBuffer<float> outputBuffer;

[numthreads(4, 1, 1)]
void computeMain(uint tid : SV_GroupIndex)
{
float4 a = float4(0);

a.zw.y = 9.0;
a.xyzw.yx = float2(1.0, 2.0);
Comment thread
jkwak-work marked this conversation as resolved.

// a = (2.0, 1.0, 0.0, 9.0)

outputBuffer[tid] = a[tid];
}
Loading