[HLSL][Driver][SPIRV] Add -fspv-use-legacy-buffer-matrix-order flag

Project: llvm/llvm-project | Language: C++ | Concepts: api-design, backwards-compatibility, refactoring | Difficulty: mid
Link: https://github.com/llvm/llvm-project/pull/214575

The problem

When using HLSL's ByteAddressBuffer::Load<T>/Store<T> with matrix types, the memory layout is ambiguous: the native layout is column-major, but legacy shaders (and DXC) assume row-major. This causes compatibility issues when porting shaders or targeting SPIR-V, where the layout expectation differs.

Why it's non-trivial

A naive fix would just flip the matrix order everywhere, but:

• The buffer API is generic and doesn't carry layout metadata.
• Only some overloads (templated ones) are affected.
• The solution must be opt-in (via a flag) for backwards compatibility.
• The transformation must only apply for SPIR-V targets and only when the flag is set.

The approach

1. Add a language option and driver flag

A new language option is introduced to control the behavior:

+LANGOPT(HLSLSpvUseLegacyBufferMatrixOrder, 1, 0, NotCompatible, "Assume the legacy matrix order (row major) when accessing raw buffers (e.g. ByteAddressBuffer). Defaults to column major.")

And a corresponding driver flag:

+def fhlsl_spv_use_legacy_buffer_matrix_order
+    : Flag<["-"], "fspv-use-legacy-buffer-matrix-order">,
+      Group<dxc_Group>,
+      Visibility<[CC1Option, DXCOption]>,
+      HelpText<"Assume the legacy matrix order (row major) when accessing "
+               "raw buffers (e.g. ByteAddressBuffer). Defaults to column "
+               "major.">,
+      MarshallingInfoFlag<LangOpts<"HLSLSpvUseLegacyBufferMatrixOrder">>;

This flag is plumbed through the driver and only allowed for SPIR-V targets:

+    if (A->getOption().getID() ==
+            options::OPT_fhlsl_spv_use_legacy_buffer_matrix_order &&
+        getArch() != llvm::Triple::spirv) {
+      getDriver().Diag(diag::err_drv_argument_only_allowed_with)
+          << A->getAsString(Args) << "-spirv";
+      A->claim();
+      continue;
+    }

2. Add a new builtin for conditional matrix transpose

A new builtin is added that will transpose its argument if it's a matrix and the flag is set, otherwise it's a no-op:

+def HLSLMaybeTransposeMatrix : LangBuiltin<"HLSL_LANG"> {
+  let Spellings = ["__builtin_hlsl_maybe_transpose_matrix"];
+  let Attributes = [NoThrow, Const, CustomTypeChecking];
+  let Prototype = "void(...)";
+}

3. Insert the builtin in ByteAddressBuffer Load/Store templated methods

When building the templated Load<T> and Store<T> methods, the code generator is instructed to call the new builtin so that the matrix is transposed if needed:

-  AddLoads("Load", AST.DependentTy); // Templated version
+  // Templated Load<T>() needs buffer-order-aware handling for matrix T.
+  AddLoads("Load", AST.DependentTy, /*TransposeResult=*/true);

And in the method body builder:

-  return MMB.dereference(PH::LastStmt).finalize();
+  MMB.dereference(PH::LastStmt);
+  if (TransposeResult)
+    MMB.callBuiltin("__builtin_hlsl_maybe_transpose_matrix", ElemTy,
+                    PH::LastStmt);
+  return MMB.finalize();

Similarly for Store<T>:

-  AddStore("Store", AST.DependentTy); // Templated version
+  // Templated Store<T>(); see addByteAddressBufferLoadMethods() above.
+  AddStore("Store", AST.DependentTy, /*TransposeArg=*/true);

And in the method body:

+  if (TransposeArg)
+    MMB.callBuiltin("__builtin_hlsl_maybe_transpose_matrix", ValueTy,
+                    PH::_1);

4. Implement the builtin's behavior in CodeGen

The code generation for the new builtin checks if the argument is a matrix and if the flag is set, and emits a transpose if so:

+  case Builtin::BI__builtin_hlsl_maybe_transpose_matrix: {
+    const Expr *ValueExpr = E->getArg(0);
+    if (hasAggregateEvaluationKind(ValueExpr->getType())) {
+      EmitAnyExprToMem(ValueExpr, ReturnValue.getAddress(),
+                       ValueExpr->getType().getQualifiers(), /*IsInit=*/true);
+      return ReturnValue.getAddress().getBasePointer();
+    }
+
+    Value *ValueOp = EmitScalarExpr(ValueExpr);
+    const auto *MatTy = ValueExpr->getType()->getAs<ConstantMatrixType>();
+    if (!MatTy || !getLangOpts().HLSLSpvUseLegacyBufferMatrixOrder)
+      return ValueOp;
+
+    unsigned Rows = MatTy->getNumRows();
+    unsigned Columns = MatTy->getNumColumns();
+    llvm::MatrixBuilder MB(Builder);
+    return MB.CreateMatrixTranspose(ValueOp, Columns, Rows);
+  }

5. Sema/type checking for the new builtin

+  case Builtin::BI__builtin_hlsl_maybe_transpose_matrix: {
+    if (SemaRef.checkArgCount(TheCall, 1))
+      return true;
+    TheCall->setType(TheCall->getArg(0)->getType());
+    break;
+  }

From the review thread

bogner: I don't really like the addition of the new builtins here, and would really prefer it if we could do this using the getpointer path. Can we represent the transpose at the HLSL level somehow and hook this in HLSLBuiltinTypeDeclBuilder directly? All the new builtins are doing is transforming the new builtins to a getpointer and an explicit transpose anyway.

hekota: What if we had a builtin that takes and returns the element value, and if it is a matrix and the flag is set, it will transpose it, otherwise it is a noop (return the same value)?

 return MMB.addParam("Index", AST.UnsignedIntTy)
      .callBuiltin("__builtin_hlsl_resource_load_typed", ElemTy, PH::Handle,
                   PH::_0, ElemTy)
      .callBuiltin("__builtin_hlsl_maybe_transpose_matrix", PH::LastStmt)
      .finalize();

Takeaways

• When adding opt-in legacy compatibility, prefer plumbing a flag through the driver and language options so the behavior is explicit and target-specific.
• For generic APIs where type information is only available late (e.g., at codegen), use a builtin or helper function to encapsulate conditional logic (like transposing matrices only when needed).
• Avoid duplicating logic across builtins; instead, create a minimal, composable builtin (like a conditional transpose) and reuse it in the method builders.