[SPIR-V] Fix matrix ordering in mul() intrinsic for vertex attributes.

[Laura Hermanns (LukasBanana)]

Vertex input attributes cannot be decorated with row_major/column_major type qualifiers, neither in HLSL nor in SPIR-V. Therefore, they cannot be emitted with a flipped matrix ordering in the mul() intrinsic that performs an implicit transpose. Instead, they must assume a flipped matrix memory layout in SPIR-V which is canceled out by keeping the intrinsic operands as-is. Note that this only works for square matrices and non-square matrices will need a separate solution.

I discovered this issue with a small HLSL shader I wrote to test indirect draw commands that generates 2D triangles and squares. This is what it looks in my D3D backend:

And this is what it looks in the Vulkan and OpenGL backends because the float2x2 rotation matrix is wrongfully transposed:

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

Input provenance is both over- and under-matched, and the symmetric vector-matrix overload remains incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes SPIR-V matrix ordering for square vertex-input matrices used in mul().

Changes:

• Adds stage-input provenance detection and adjusted matrix-vector lowering.
• Adds a float2x2 regression test.
• Corrects a lit configuration exception.

File summaries

File	Description
SpirvEmitter.cpp	Adjusts SPIR-V mul() lowering.
vertex.attribute.float2x2.mul.intrinsic.hlsl	Tests vertex-input matrix multiplication.
TestingConfig.py	Raises a valid RuntimeError.

Review details

Suppressed comments (1)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:643

• This provenance walk loses the input-layout state after a local assignment. Function-local initialization is emitted as an OpStore followed by a later OpLoad from Function storage (see this file at lines 2297-2304), so float2x2 r = input.rotation2D; mul(r, v) falls through the default path and remains incorrectly transposed. The layout-origin state needs to survive local copies/loads.

        case SpirvInstruction::IK_Load:
            return originatesFromInputStorage(cast<SpirvLoad>(inst)->getPointer());

• Files reviewed: 3/3 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Input provenance is misclassified or lost in several cases, and the reversed multiplication overload remains incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:604

• A semantic on a member does not establish that the expression is a stage input. Output structs also use semantic-decorated matrix members (for example, spirv.stage-io.relaxed-precision.hlsl has a matrix VSOut::outA), so mul(output.outA, v) is now classified as input-backed and gets the opposite multiplication order. Determine direction from the root entry-point parameter or tracked storage provenance instead of returning true for any semantic-bearing member.

    if (const auto *member = dyn_cast<MemberExpr>(expr)) {
      if (const auto *decl = dyn_cast<DeclaratorDecl>(member->getMemberDecl()))
        if (hasAnySemantic(decl))
          return true;

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12453

• This provenance check loses the condition as soon as the attribute is copied to a local variable or passed as a plain matrix parameter: isVertexInputExpr(localMatrix) is false and its SPIR-V load points to Function storage. Consequently, float2x2 m = input.rotation2D; mul(m, v) still takes the old, incorrect path. Preserve/normalize the matrix ordering when loading stage input, or propagate explicit provenance through stores and calls.

        const bool isSquare = (numRows == numCols);
        const bool fromVertexInput =
          isVertexInputExpr(arg0) || originatesFromInputStorage(arg0Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12459

• The equivalent input-matrix case with the operands reversed is still incorrect. mul(v, input.rotation2D) exits through the earlier vector/matrix branch and emits OpMatrixTimesVector, even though the same unflipped vertex-attribute layout requires keeping those operands as-is with OpVectorTimesMatrix for a square matrix. Apply the special handling symmetrically and add coverage for this overload.

        // Workaround: if matrix originates from vertex input and is square,
        // emit OpMatrixTimesVector without operand swapping.
        // Otherwise, row_major cannot be emulated here because
        // SPIR-V vertex attributes cannot be decorated with row_major layout.
        if (isSquare && fromVertexInput)

tools/clang/test/CodeGenSPIRV/vertex.attribute.float2x2.mul.intrinsic.hlsl:19

• Correct the typo “emited” to “emitted.”

float4 main(VSIn input) : SV_Position {

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Input provenance is both over-broad and easily lost, while the equivalent vector-matrix overload remains unfixed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12453

• This loses provenance when the attribute is copied to a local or passed to a helper: the AST argument then has no semantic, while its SPIR-V load points to Function storage. For example, float2x2 r = input.rotation2D; mul(r, p) still takes the old, incorrect OpVectorTimesMatrix path. Preserve the matrix-layout provenance through stores/parameters or normalize it when loading stage inputs.

        const bool fromVertexInput =
          isVertexInputExpr(arg0) || originatesFromInputStorage(arg0Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12453

• The corresponding mul(vector, matrix) overload above is left unchanged. When the matrix is a vertex attribute, mul(input.position2D, input.rotation2D) has the same layout mismatch but still emits OpMatrixTimesVector; that branch also needs to preserve operand order (using OpVectorTimesMatrix) and be covered by the regression test.

        const bool isSquare = (numRows == numCols);
        const bool fromVertexInput =
          isVertexInputExpr(arg0) || originatesFromInputStorage(arg0Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12459

• This is a user-visible SPIR-V code-generation correctness fix, so the release-note policy in CONTRIBUTING.md calls for an entry under docs/ReleaseNotes.md → “Upcoming Release” → “Bug Fixes”. Please add one, or point to planned shared coverage if this is part of a multi-PR effort.

        // Workaround: if matrix originates from vertex input and is square,
        // emit OpMatrixTimesVector without operand swapping.
        // Otherwise, row_major cannot be emulated here because
        // SPIR-V vertex attributes cannot be decorated with row_major layout.
        if (isSquare && fromVertexInput)

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Input detection affects non-vertex stages and loses provenance across aliases and helper calls.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12434

• isVertexInputExpr and originatesFromInputStorage recognize inputs from every shader stage, so this path also changes square-matrix multiplication for pixel/hull/domain/geometry inputs. Those are inter-stage values rather than vertex-buffer attributes, and their producer and consumer already share the normal SPIR-V matrix representation; applying the no-swap workaround there changes the result. Restrict this condition to vertex shaders.

        const bool fromVertexInput =
            isVertexInputExpr(arg1) || originatesFromInputStorage(arg1Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12468

• This predicate also matches semantic-bearing inputs and Input storage from non-vertex stages. For example, a pixel-shader matrix input would now use the vertex-attribute workaround even though it came from a preceding shader stage, producing the opposite multiplication. Gate this branch on the vertex shader stage as well.

        const bool fromVertexInput =
            isVertexInputExpr(arg0) || originatesFromInputStorage(arg0Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12443

• The new test exercises only the mul(matrix, vector) workaround, while this separately changed mul(vector, matrix) branch is untested. Add a vertex-input case such as mul(input.position2D, input.rotation2D) and verify that it emits OpVectorTimesMatrix with operands kept in source order.

        if (isSquare && fromVertexInput)
          return spvBuilder.createBinaryOp(spv::Op::OpVectorTimesMatrix,
                                           returnType, arg0Id, arg1Id, loc,
                                           range);

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Provenance tracking is incomplete and path-insensitive, causing incorrect lowering for transformed, mixed-source, and matrix-matrix values.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:3484

• vertexInputParams is keyed only by the parameter declaration and OR-ed across every call site. If one helper is called with both a vertex-input matrix and a cbuffer/local matrix, its single emitted body sees this bit as true and uses vertex ordering for both calls, miscompiling the non-input call. Provenance cannot be a declaration-wide property; normalize the representation at the call boundary or specialize the helper by representation.

      if (rhsVal) {
        tempVar->setFromVertexInput(rhsVal->isFromVertexInput());

tools/clang/lib/SPIRV/SpirvEmitter.cpp:7381

• This mutable flag is not control-flow aware. For a local matrix assigned from a cbuffer on one branch and from a vertex attribute on another, the flag reflects whichever store the emitter visits last, although either value can reach the later mul() at runtime; one branch will therefore use the wrong matrix operation. Normalize matrix representation at stores/merges or track it per SSA value instead of mutating variable metadata.

    return;

tools/clang/lib/SPIRV/SpirvEmitter.cpp:657

• The provenance walk drops matrix values produced by unlisted instructions. For example, mul(input.rotation + 0, position) produces an IK_BinaryOp, reaches this default, and misses the special path even though the value still has vertex-input layout; function calls, selects, and composite reconstruction have the same gap. Propagate or normalize the representation across all matrix-producing operations and add regression cases for transformed values.

    default:
      return false;

• Files reviewed: 8/8 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Provenance is lost or conflated across local loads, control-flow stores, and shared function calls, causing incorrect lowering.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:1462

• createLoad already copies provenance from the pointer, but this assignment replaces it with the AST-only result. Loading a local initialized from a vertex attribute therefore clears the bit, so copying or passing that local loses its origin and a later mul() takes the old transposed path. Preserve both sources of provenance.

  loadedInstr->setFromVertexInput(loadedInstr->isFromVertexInput() ||

tools/clang/lib/SPIRV/SpirvEmitter.cpp:3485

• This folds all call sites into one bit on the shared ParmVarDecl, although the callee body is emitted only once. If a helper is called with both a vertex-input matrix and a cbuffer/local matrix, one opcode convention is selected for both calls, making one result incorrect; queue order can also discover an input call after the callee has already been emitted. The matrix representation needs to be normalized at the input boundary or the callee must be specialized per representation instead of storing call-site provenance globally.

        tempVar->setFromVertexInput(rhsVal->isFromVertexInput());

tools/clang/lib/SPIRV/SpirvEmitter.cpp:7383

• This metadata update is path-insensitive: every dynamic store mutates the same compile-time pointer object, so the last assignment visited by codegen determines all later loads. For a local assigned a vertex matrix in one branch and a cbuffer matrix in another, the subsequent mul() is emitted with one convention and is wrong on one runtime path. A single mutable boolean cannot represent mixed-origin control flow; normalize the representation before merging values instead.
  tools/clang/lib/SPIRV/SpirvEmitter.cpp:12454
• The new regression test exercises only the mul(matrix, vector) workaround below; this independent mul(vector, matrix) branch is untested. Please add a square vertex-input matrix case that checks for OpVectorTimesMatrix with the operands left in source order.

        // SPIR-V vertex attributes cannot be decorated with row_major layout.
        if (isSquare && fromVertexInput)
          return spvBuilder.createBinaryOp(spv::Op::OpVectorTimesMatrix,
                                           returnType, arg0Id, arg1Id, loc,

docs/ReleaseNotes.md:36

• Correct the misspelling of “matrices” in this user-facing release note.

- SPIR-V: Fixed matrix ordering for vertex input attributes of square martices.

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Call-insensitive and control-flow-insensitive provenance tracking can select incorrect matrix multiplication ordering.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

docs/ReleaseNotes.md:36

• Correct the typo in the release note.

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12455

• The new test covers only mul(matrix, vector), leaving this separately changed mul(vector, matrix) vertex-input path untested. Add a square vertex-input matrix case with the vector as the first operand and check that it emits OpVectorTimesMatrix without swapping operands.

        if (isSquare && fromVertexInput)
          return spvBuilder.createBinaryOp(spv::Op::OpVectorTimesMatrix,
                                           returnType, arg0Id, arg1Id, loc,
                                           range);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12457

• This comment names the opposite instruction from the implementation below: the default vector/matrix path emits OpMatrixTimesVector, not OpVectorTimesMatrix.

        // Default path (existing behavior): swap operands and emit VectorTimesMatrix.

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Provenance is lost or conflated across helper calls and control-flow joins, causing incorrect lowering in supported scenarios.

Review details

Suppressed comments (4)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:3486

• This declaration-level OR conflates all call sites. A helper invoked once with a vertex-input matrix and once with a cbuffer/local matrix is emitted only once, and this bit makes every mul in that helper use the vertex-input lowering, so the non-input call is transposed incorrectly. Provenance needs to remain call-site/value-specific (or the callee must be specialized/inputs normalized) rather than accumulated per ParmVarDecl.

        vertexInputParams[param] |= rhsVal->isFromVertexInput();

tools/clang/lib/SPIRV/SpirvEmitter.cpp:657

• Function-call results lose the new provenance: createFunctionCall does not set the flag, and IK_FunctionCall falls through here. Consequently, a square matrix returned unchanged from a helper (for example, mul(passThrough(input.rotation), v)) still takes the old swapped-operand path. Propagate return-value provenance through calls or normalize the matrix representation at the function boundary.

    default:
      return false;

docs/ReleaseNotes.md:36

• Correct the typo in the release note: “martices” should be “matrices.”

- SPIR-V: Fixed matrix ordering for vertex input attributes of square martices.

tools/clang/lib/SPIRV/SpirvEmitter.cpp:7384

• This mutable flag is overwritten in source traversal order rather than merged according to control flow. If one branch assigns a vertex-input matrix and another assigns a normal matrix to the same local, the final visited store determines the single lowering used after the join, making one runtime branch incorrect. Normalize representations before the merge or track provenance in a control-flow-aware value analysis.

  lhsPtr->setFromVertexInput(rhsVal->isFromVertexInput());

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Provenance tracking currently produces incorrect lowering for mixed call sites, control-flow merges, helper parameters, and transformed matrices.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:657

• The provenance walk drops input origin for common value-producing operations not listed here. For example, scaling an attribute matrix before mul produces a binary instruction, and returning it through a helper produces a function-call instruction; both then fall through to false and reintroduce the original ordering bug. Propagate provenance through all representation-preserving operations, with tests for transformed and returned matrices.

    case SpirvInstruction::IK_UnaryOp:
      return originatesFromInputStorage(cast<SpirvUnaryOp>(inst)->getOperand());
    default:
      return false;

tools/clang/lib/SPIRV/SpirvEmitter.cpp:3486

• This ORs provenance into one global bit per callee parameter, although a function can be called with both a vertex-input matrix and a regular matrix. The function body is emitted only once, so the vertex-input opcode is then used for every call and miscompiles the regular argument; work-queue ordering can also let later-discovered callers update this map after the body was emitted. The lowering needs call-context specialization or a normalized matrix representation rather than a callee-wide boolean.

        vertexInputParams[param] |= rhsVal->isFromVertexInput();

tools/clang/lib/SPIRV/SpirvEmitter.cpp:7384

• Mutating a storage instruction with the provenance of the most recently emitted store is not control-flow-sensitive. After a conditional assignment from a vertex input, a later mul is always lowered as vertex-derived even on runtime paths where the variable still contains a regular matrix (and the reverse depends on traversal order). Normalize the matrix when storing it or track provenance per SSA value/control-flow merge instead of on the mutable pointer.

  lhsPtr->setFromVertexInput(rhsVal->isFromVertexInput());

docs/ReleaseNotes.md:36

• Correct the typo “martices” to “matrices.”

- SPIR-V: Fixed matrix ordering for vertex input attributes of square martices.

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Input detection affects non-vertex stages, misclassifies helper parameters, and loses provenance through local variables.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

tools/clang/lib/SPIRV/SpirvEmitter.cpp:654

• Provenance stops at function-local storage. For example, after float2x2 m = input.rotation;, mul(m, v) has an AST root that is a local VarDecl and an instruction that loads from Function storage, so this returns false and the original ordering bug remains. Track the value stored into local variables (and subsequent copies) or normalize the stage-input representation when it is loaded.

    default:
      return false;

tools/clang/lib/SPIRV/SpirvEmitter.cpp:12468

• The same missing stage check affects the matrix/vector overload: semantic-bearing or Input-storage matrices in non-vertex stages are incorrectly treated as vertex attributes. Restrict this workaround to vertex shaders so inter-stage matrix inputs retain their established lowering.

        const bool fromVertexInput =
            isVertexInputExpr(arg0) || originatesFromInputStorage(arg0Id);

tools/clang/lib/SPIRV/SpirvEmitter.cpp:623

• This does not establish that parm is an entry-point input. Any ordinary helper parameter whose struct type contains semantic-bearing fields satisfies this condition, even when the caller passes locally or buffer-created data, so mul() in that helper can incorrectly use the vertex-layout workaround. Track the matrix layout through function arguments or otherwise distinguish the actual entry parameter from helper parameters.

    if (const auto *declRef = dyn_cast<DeclRefExpr>(expr)) {
      if (const auto *parm = dyn_cast<ParmVarDecl>(declRef->getDecl()))
        return canActAsInParmVar(parm) &&
               (hasSemanticField || hasAnySemantic(parm));

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

[Laura Hermanns (LukasBanana)]

I tried to wrangle between Claude's output and Copilot's feedback. I'm tired and will use a workaround in my compiler toolchain now :)