llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-codegen Author: Farzon Lotfi (farzonl) <details> <summary>Changes</summary> fixes https://github.com/llvm/llvm-project/issues/136941 This flag, mirroring DXC, opts into assuming row-major buffer bytes instead, for compatibility with legacy shaders. Outside of the driver change the raw buffers carry no layout metadata, so matrix values loaded/stored via ByteAddressBuffer::Load<T>/Store<T> are ambiguous (native layout is column-major). - New __builtin_hlsl_resource_load_typed/store_typed builtins for the templated ByteAddressBuffer Load<T>/Store<T> overloads - CodeGen transposes matrix values via llvm.matrix.transpose when the flag is set and the target is SPIR-V - Plumb flag from driver -> cc1 -> new LangOpts bit Assisted by Claude Sonnet 5 via CoPilot --- Full diff: https://github.com/llvm/llvm-project/pull/214575.diff 11 Files Affected: - (modified) clang/include/clang/Basic/Builtins.td (+6) - (modified) clang/include/clang/Basic/LangOptions.def (+1) - (modified) clang/include/clang/Options/Options.td (+9) - (modified) clang/lib/CodeGen/CGHLSLBuiltins.cpp (+15) - (modified) clang/lib/Driver/ToolChains/Clang.cpp (+1) - (modified) clang/lib/Driver/ToolChains/HLSL.cpp (+9) - (modified) clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp (+65-2) - (modified) clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.h (+4) - (modified) clang/lib/Sema/SemaHLSL.cpp (+10) - (added) clang/test/CodeGenHLSL/resources/ByteAddressBuffers-matrix-legacy-order.hlsl (+37) - (added) clang/test/Driver/dxc_fspv_use_legacy_buffer_matrix_order.hlsl (+17) ``````````diff diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td index b67a22ad50689..b2e6f4fe8cd25 100644 --- a/clang/include/clang/Basic/Builtins.td +++ b/clang/include/clang/Basic/Builtins.td @@ -5317,6 +5317,12 @@ def HLSLResourceGetPointerTyped : Builtin { let Prototype = "void(...)"; } +def HLSLMaybeTransposeMatrix : LangBuiltin<"HLSL_LANG"> { + let Spellings = ["__builtin_hlsl_maybe_transpose_matrix"]; + let Attributes = [NoThrow, Const, CustomTypeChecking]; + let Prototype = "void(...)"; +} + def HLSLResourceLoadWithStatus : LangBuiltin<"HLSL_LANG"> { let Spellings = ["__builtin_hlsl_resource_load_with_status"]; let Attributes = [NoThrow]; diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def index f945b60b6966d..ad993ce7e5d95 100644 --- a/clang/include/clang/Basic/LangOptions.def +++ b/clang/include/clang/Basic/LangOptions.def @@ -250,6 +250,7 @@ ENUM_LANGOPT(HLSLVersion, HLSLLangStd, 16, HLSL_Unset, NotCompatible, "HLSL Vers LANGOPT(HLSLStrictAvailability, 1, 0, NotCompatible, "Strict availability diagnostic mode for HLSL built-in functions.") LANGOPT(HLSLSpvUseUnknownImageFormat, 1, 0, NotCompatible, "For storage images and texel buffers, sets the default format to 'Unknown' when not specified via the `vk::image_format` attribute. If this option is not used, the format is inferred from the resource's data type.") +LANGOPT(HLSLSpvUseLegacyBufferMatrixOrder, 1, 0, NotCompatible, "Assume the legacy matrix order (row major) when accessing raw buffers (e.g. ByteAddressBuffer). Defaults to column major.") LANGOPT(HLSLSpvEnableMaximalReconvergence, 1, 0, NotCompatible, "Enables the MaximallyReconvergesKHR execution mode for this module. This ensures that control flow reconverges at well-defined merge points as defined by the Vulkan spec.") LANGOPT(HLSLSpvPreserveInterface, 1, 0, NotCompatible, "Preserve entry-point interface variables from dead-code elimination.") LANGOPT(EmitLogicalPointer, 1, 0, NotCompatible, "Allow emitting structured GEP/alloca intrinsics instead of normal GEP/alloca instructions.") diff --git a/clang/include/clang/Options/Options.td b/clang/include/clang/Options/Options.td index b354a475346df..6fdb41a63c4be 100644 --- a/clang/include/clang/Options/Options.td +++ b/clang/include/clang/Options/Options.td @@ -10006,6 +10006,15 @@ def fhlsl_spv_use_unknown_image_format "from the resource's data type.">, MarshallingInfoFlag<LangOpts<"HLSLSpvUseUnknownImageFormat">>; +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">>; + def fhlsl_spv_enable_maximal_reconvergence : Flag<["-"], "fspv-enable-maximal-reconvergence">, Group<dxc_Group>, diff --git a/clang/lib/CodeGen/CGHLSLBuiltins.cpp b/clang/lib/CodeGen/CGHLSLBuiltins.cpp index c7d1de29f0651..e5f55c5c6d348 100644 --- a/clang/lib/CodeGen/CGHLSLBuiltins.cpp +++ b/clang/lib/CodeGen/CGHLSLBuiltins.cpp @@ -647,6 +647,21 @@ Value *CodeGenFunction::EmitHLSLBuiltinExpr(unsigned BuiltinID, CI->setCallingConv(IntrFn->getCallingConv()); return CI; } + case Builtin::BI__builtin_hlsl_maybe_transpose_matrix: { + Value *ValueOp = EmitScalarExpr(E->getArg(0)); + const auto *MatTy = E->getArg(0)->getType()->getAs<ConstantMatrixType>(); + if (!MatTy || !CGM.getTriple().isSPIRV() || + !getLangOpts().HLSLSpvUseLegacyBufferMatrixOrder) + return ValueOp; + + bool IsLoad = + E->getArg(1)->EvaluateKnownConstInt(getContext()).getBoolValue(); + unsigned Rows = MatTy->getNumRows(); + unsigned Columns = MatTy->getNumColumns(); + llvm::MatrixBuilder MB(Builder); + return IsLoad ? MB.CreateMatrixTranspose(ValueOp, Columns, Rows) + : MB.CreateMatrixTranspose(ValueOp, Rows, Columns); + } case Builtin::BI__builtin_hlsl_resource_sample: { Value *HandleOp = EmitScalarExpr(E->getArg(0)); Value *SamplerOp = EmitScalarExpr(E->getArg(1)); diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index d2e22920aa432..8e5d54cf16430 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -3949,6 +3949,7 @@ static void RenderHLSLOptions(const Driver &D, const ArgList &Args, options::OPT_fdx_rootsignature_define, options::OPT_fdx_rootsignature_version, options::OPT_fhlsl_spv_use_unknown_image_format, + options::OPT_fhlsl_spv_use_legacy_buffer_matrix_order, options::OPT_fhlsl_spv_enable_maximal_reconvergence, options::OPT_fhlsl_spv_preserve_interface}; if (!types::isHLSL(InputType)) diff --git a/clang/lib/Driver/ToolChains/HLSL.cpp b/clang/lib/Driver/ToolChains/HLSL.cpp index 558408cc969f0..9e7005b8798f2 100644 --- a/clang/lib/Driver/ToolChains/HLSL.cpp +++ b/clang/lib/Driver/ToolChains/HLSL.cpp @@ -533,6 +533,15 @@ HLSLToolChain::TranslateArgs(const DerivedArgList &Args, BoundArch BA, continue; } + 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; + } + if (A->getOption().getID() == options::OPT_enable_16bit_types) { // Translate -enable-16bit-types into -fnative-half-type and // -fnative-int16-type diff --git a/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp b/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp index 929e8bc824afa..048c47f76f7bf 100644 --- a/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp +++ b/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.cpp @@ -1635,7 +1635,15 @@ BuiltinTypeDeclBuilder::addByteAddressBufferLoadMethods() { AddLoads("Load2", AST.getExtVectorType(AST.UnsignedIntTy, 2)); AddLoads("Load3", AST.getExtVectorType(AST.UnsignedIntTy, 3)); AddLoads("Load4", AST.getExtVectorType(AST.UnsignedIntTy, 4)); - AddLoads("Load", AST.DependentTy); // Templated version + + // Templated Load<T>() needs buffer-order-aware handling for matrix T. + { + IdentifierInfo &II = AST.Idents.get("Load", tok::TokenKind::identifier); + DeclarationName Load(&II); + addRawBufferGenericLoadFunction(Load); + addLoadWithStatusFunction(Load, AST.DependentTy); + } + return *this; } @@ -1656,7 +1664,13 @@ BuiltinTypeDeclBuilder::addByteAddressBufferStoreMethods() { AddStore("Store2", AST.getExtVectorType(AST.UnsignedIntTy, 2)); AddStore("Store3", AST.getExtVectorType(AST.UnsignedIntTy, 3)); AddStore("Store4", AST.getExtVectorType(AST.UnsignedIntTy, 4)); - AddStore("Store", AST.DependentTy); // Templated version + + // Templated Store<T>(); see addByteAddressBufferLoadMethods() above. + { + IdentifierInfo &II = AST.Idents.get("Store", tok::TokenKind::identifier); + DeclarationName Store(&II); + addRawBufferGenericStoreFunction(Store); + } return *this; } @@ -2372,6 +2386,55 @@ BuiltinTypeDeclBuilder::addLoadWithStatusFunction(DeclarationName &Name, return MMB.finalize(); } +BuiltinTypeDeclBuilder & +BuiltinTypeDeclBuilder::addRawBufferGenericLoadFunction(DeclarationName &Name) { + assert(!Record->isCompleteDefinition() && "record is already complete"); + ASTContext &AST = SemaRef.getASTContext(); + using PH = BuiltinTypeMethodBuilder::PlaceHolder; + + // The empty QualType is a placeholder. The actual return type is set below + // once the template parameter is created. This method is always const; + // it does not rebind the resource handle. + BuiltinTypeMethodBuilder MMB(*this, Name, QualType(), /*IsConst=*/true); + QualType ElemTy = MMB.addTemplateTypeParam("element_type"); + MMB.ReturnTy = ElemTy; + QualType AddrSpaceElemTy = + AST.getAddrSpaceQualType(ElemTy, LangAS::hlsl_device); + QualType ElemPtrTy = AST.getPointerType(AddrSpaceElemTy); + + return MMB.addParam("Index", AST.UnsignedIntTy) + .callBuiltin("__builtin_hlsl_resource_getpointer_typed", ElemPtrTy, + PH::Handle, PH::_0, ElemTy) + .dereference(PH::LastStmt) + .callBuiltin("__builtin_hlsl_maybe_transpose_matrix", ElemTy, + PH::LastStmt, getConstantIntExpr(1)) + .finalize(); +} + +BuiltinTypeDeclBuilder & +BuiltinTypeDeclBuilder::addRawBufferGenericStoreFunction( + DeclarationName &Name) { + assert(!Record->isCompleteDefinition() && "record is already complete"); + ASTContext &AST = SemaRef.getASTContext(); + using PH = BuiltinTypeMethodBuilder::PlaceHolder; + + BuiltinTypeMethodBuilder MMB(*this, Name, AST.VoidTy, /*IsConst=*/false); + QualType ElemTy = MMB.addTemplateTypeParam("element_type"); + QualType AddrSpaceElemTy = + AST.getAddrSpaceQualType(ElemTy, LangAS::hlsl_device); + QualType ElemPtrTy = AST.getPointerType(AddrSpaceElemTy); + + return MMB.addParam("Index", AST.UnsignedIntTy) + .addParam("Value", ElemTy) + .callBuiltin("__builtin_hlsl_maybe_transpose_matrix", ElemTy, PH::_1, + getConstantIntExpr(0)) + .callBuiltin("__builtin_hlsl_resource_getpointer_typed", ElemPtrTy, + PH::Handle, PH::_0, ElemTy) + .dereference(PH::LastStmt) + .assign(PH::LastStmt, PH::LastStmt) + .finalize(); +} + BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addHandleAccessFunction( DeclarationName &Name, bool IsConstReturn, bool IsRef, QualType IndexTy, QualType ElemTy) { diff --git a/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.h b/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.h index afb336045b1f7..5eb5e7ecad69d 100644 --- a/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.h +++ b/clang/lib/Sema/HLSLBuiltinTypeDeclBuilder.h @@ -136,6 +136,10 @@ class BuiltinTypeDeclBuilder { BuiltinTypeDeclBuilder &addStoreFunction(DeclarationName &Name, bool IsConst, QualType ValueType); BuiltinTypeDeclBuilder & + addRawBufferGenericLoadFunction(DeclarationName &Name); + BuiltinTypeDeclBuilder & + addRawBufferGenericStoreFunction(DeclarationName &Name); + BuiltinTypeDeclBuilder & addByteAddressBufferInterlockedMethod(StringRef MethodName, QualType ValueTy, StringRef BuiltinName); BuiltinTypeDeclBuilder &addAppendMethod(); diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp index 3b9d9e4ed964b..4fe65a06631bf 100644 --- a/clang/lib/Sema/SemaHLSL.cpp +++ b/clang/lib/Sema/SemaHLSL.cpp @@ -4221,6 +4221,16 @@ bool SemaHLSL::CheckBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) { break; } + case Builtin::BI__builtin_hlsl_maybe_transpose_matrix: { + if (SemaRef.checkArgCount(TheCall, 2) || + CheckArgTypeMatches(&SemaRef, TheCall->getArg(1), + SemaRef.getASTContext().IntTy)) + return true; + + TheCall->setType(TheCall->getArg(0)->getType()); + + break; + } case Builtin::BI__builtin_hlsl_resource_load_with_status: { if (SemaRef.checkArgCount(TheCall, 3) || CheckResourceHandle(&SemaRef, TheCall, 0) || diff --git a/clang/test/CodeGenHLSL/resources/ByteAddressBuffers-matrix-legacy-order.hlsl b/clang/test/CodeGenHLSL/resources/ByteAddressBuffers-matrix-legacy-order.hlsl new file mode 100644 index 0000000000000..e1ee18a697373 --- /dev/null +++ b/clang/test/CodeGenHLSL/resources/ByteAddressBuffers-matrix-legacy-order.hlsl @@ -0,0 +1,37 @@ +// RUN: %clang_cc1 -triple spirv-unknown-vulkan-library -finclude-default-header \ +// RUN: -emit-llvm -disable-llvm-passes -o - %s | FileCheck %s --check-prefixes=CHECK,DEFAULT +// RUN: %clang_cc1 -triple spirv-unknown-vulkan-library -finclude-default-header \ +// RUN: -fspv-use-legacy-buffer-matrix-order -emit-llvm -disable-llvm-passes -o - %s \ +// RUN: | FileCheck %s --check-prefixes=CHECK,LEGACY + +// Raw buffers carry no layout information. By default, a matrix read from or +// written to a raw buffer is assumed to be stored in column-major order, +// matching the native in-register representation, so no reordering is +// needed. -fspv-use-legacy-buffer-matrix-order assumes the raw bytes are +// stored in row-major order instead, which requires transposing the loaded +// (or, before storing, the to-be-stored) value. + +ByteAddressBuffer Buf : register(t0); +RWByteAddressBuffer RWBuf : register(u0); + +export float2x3 TestLoad() { + return Buf.Load<float2x3>(0); +} + +// CHECK-LABEL: define {{.*}} <6 x float> @{{.*}}ByteAddressBuffer4LoadIu11matrix_typeILj2ELj3EfEEET_j +// CHECK: [[LOADED:%.*]] = load <6 x float>, ptr addrspace(11) %{{.*}} +// DEFAULT-NOT: call {{.*}} @llvm.matrix.transpose +// DEFAULT: ret <6 x float> [[LOADED]] +// LEGACY: [[TRANSPOSED:%.*]] = call {{.*}} <6 x float> @llvm.matrix.transpose.v6f32(<6 x float> [[LOADED]], i32 3, i32 2) +// LEGACY: ret <6 x float> [[TRANSPOSED]] + +export void TestStore(float2x3 M) { + RWBuf.Store<float2x3>(0, M); +} + +// CHECK-LABEL: define {{.*}} void @{{.*}}RWByteAddressBuffer5StoreIu11matrix_typeILj2ELj3EfEEEvjT_ +// CHECK: [[VALUE:%.*]] = load <6 x float>, ptr %Value.addr +// DEFAULT-NOT: call {{.*}} @llvm.matrix.transpose +// DEFAULT: store <6 x float> [[VALUE]], ptr addrspace(11) %{{.*}} +// LEGACY: [[TRANSPOSED:%.*]] = call {{.*}} <6 x float> @llvm.matrix.transpose.v6f32(<6 x float> [[VALUE]], i32 2, i32 3) +// LEGACY: store <6 x float> [[TRANSPOSED]], ptr addrspace(11) %{{.*}} diff --git a/clang/test/Driver/dxc_fspv_use_legacy_buffer_matrix_order.hlsl b/clang/test/Driver/dxc_fspv_use_legacy_buffer_matrix_order.hlsl new file mode 100644 index 0000000000000..f63985c7745a6 --- /dev/null +++ b/clang/test/Driver/dxc_fspv_use_legacy_buffer_matrix_order.hlsl @@ -0,0 +1,17 @@ +// Verify that -fspv-use-legacy-buffer-matrix-order is accepted by the driver +// and forwarded to cc1 as -fspv-use-legacy-buffer-matrix-order. +// RUN: %clang_dxc -spirv -Tlib_6_7 -fspv-use-legacy-buffer-matrix-order -### %s 2>&1 \ +// RUN: | FileCheck %s --check-prefix=CHECK-LEGACY +// CHECK-LEGACY: "-fspv-use-legacy-buffer-matrix-order" + +// Without the flag, -fspv-use-legacy-buffer-matrix-order must not appear in +// cc1 args. +// RUN: %clang_dxc -spirv -Tlib_6_7 -### %s 2>&1 \ +// RUN: | FileCheck %s --check-prefix=CHECK-NO-LEGACY +// CHECK-NO-LEGACY-NOT: "-fspv-use-legacy-buffer-matrix-order" + +// The flag requires -spirv. +// RUN: not %clang_dxc -Tlib_6_7 -fspv-use-legacy-buffer-matrix-order -### %s 2>&1 \ +// RUN: | FileCheck %s --check-prefix=CHECK-NO-SPIRV +// CHECK-NO-SPIRV: error: invalid argument '-fspv-use-legacy-buffer-matrix-order' only allowed with '-spirv' + `````````` </details> https://github.com/llvm/llvm-project/pull/214575 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
