Explicit types in cbuffer layouts #156919
Draft
+639
−624
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CHECK-lines ignore whitespace, so we can remove some here and make this a bit easier to read.
This mostly just works. A couple of notes: 1. We should use explicit padding 2. A couple of the tests have hacks to deal with vector alignment 3. More complicated GEPs probably don't have coverage
This abandons the `dx.Layout` idea and just uses explicit padding. Note: this doesn't really work as is. Since the AST still has everything represented as standard types without the padding, various places will create GEPs with the wrong offsets and we fail to clean these up. We need to move some of this logic into Sema and the AST itself. There are a couple of other things on top of that: - We can't/shouldn't just use `i8` arrays - we need something explicit. - The algorithm to add padding in HLSLBufferLayoutBuilder is hacky. - Reordered fields break stuff, including ones from implicit bindings.
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,c,cpp -- clang/lib/Basic/Targets/DirectX.h clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/CGExprAgg.cpp clang/lib/CodeGen/CGHLSLRuntime.cpp clang/lib/CodeGen/CGHLSLRuntime.h clang/lib/CodeGen/HLSLBufferLayoutBuilder.cpp clang/lib/CodeGen/HLSLBufferLayoutBuilder.h clang/lib/CodeGen/Targets/DirectX.cpp clang/lib/CodeGen/Targets/SPIR.cpp clang/test/CodeGenHLSL/basic-target.c llvm/include/llvm/Analysis/DXILResource.h llvm/lib/Analysis/DXILResource.cpp llvm/lib/Frontend/HLSL/CBuffer.cpp llvm/lib/Target/DirectX/DXILCBufferAccess.cpp llvm/lib/Target/DirectX/DirectXTargetMachine.cpp
View the diff from clang-format here.diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 42687a94d..2b7d488af 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4682,7 +4682,7 @@ LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
} else if (E->getType().getAddressSpace() == LangAS::hlsl_constant) {
// This is an array inside of a cbuffer.
Addr = EmitPointerWithAlignment(E->getBase(), &EltBaseInfo, &EltTBAAInfo);
- auto *Idx = EmitIdxAfterBase(/*Promote*/true);
+ auto *Idx = EmitIdxAfterBase(/*Promote*/ true);
// ...
CharUnits RowAlignedSize = getContext()
@@ -4694,7 +4694,7 @@ LValue CodeGenFunction::EmitArraySubscriptExpr(const ArraySubscriptExpr *E,
llvm::Value *ScaledIdx = Builder.CreateMul(Idx, RowAlignedSizeVal);
CharUnits EltAlign =
- getArrayElementAlign(Addr.getAlignment(), Idx, RowAlignedSize);
+ getArrayElementAlign(Addr.getAlignment(), Idx, RowAlignedSize);
llvm::Value *EltPtr =
emitArraySubscriptGEP(*this, Int8Ty, Addr.emitRawPointer(*this),
ScaledIdx, false, SignedIndices, E->getExprLoc());
|
Icohedron
reviewed
Sep 4, 2025
| const unsigned IndexInLayout = IndexInLayoutElements + 1; | ||
| assert(Layout[IndexInLayout] == UINT_MAX && | ||
| LayoutElements[IndexInLayoutElements] == nullptr); | ||
| assert(Layout[I.second] == std::pair(UINT_MAX, nullptr)); |
There was a problem hiding this comment.
This line causes a build failure on my machine:
[264/1543] Building CXX object tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/HLSLBufferLayoutBuilder.cpp.o
FAILED: tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/HLSLBufferLayoutBuilder.cpp.o
/nix/store/bichdz31xjvg04ajh8b7qjw6q704a2lb-sccache-0.10.0/bin/sccache /nix/store/pprbhrczrj2gbvvsb0fr2dpqahqz6m77-clang-wrapper-19.1.7/bin/clang++ -DCLANG_EXPORTS -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GLIBCXX_USE_CXX11_ABI=1 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/workspace/llvm-project/build/tools/clang/lib/CodeGen -I/workspace/llvm-project/clang/lib/CodeGen -I/workspace/llvm-project/clang/include -I/workspace/llvm-project/build/tools/clang/include -I/workspace/llvm-project/build/include -I/workspace/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O2 -g -DNDEBUG -std=c++17 -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/HLSLBufferLayoutBuilder.cpp.o -MF tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/HLSLBufferLayoutBuilder.cpp.o.d -o tools/clang/lib/CodeGen/CMakeFiles/obj.clangCodeGen.dir/HLSLBufferLayoutBuilder.cpp.o -c /workspace/llvm-project/clang/lib/CodeGen/HLSLBufferLayoutBuilder.cpp
/workspace/llvm-project/clang/lib/CodeGen/HLSLBufferLayoutBuilder.cpp:136:29: error: invalid operands to binary expression ('std::pair<unsigned int, llvm::Type *>' and 'std::pair<unsigned int, std::nullptr_t>' (aka 'pair<unsigned int, std::nullptr_t>'))
136 | assert(Layout[I.second] == std::pair(UINT_MAX, nullptr));
| ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Icohedron
reviewed
Sep 4, 2025
There was a problem hiding this comment.
This test is failing for me locally. It appears that some memcpy intrinsics are being replaced with loads and stores.
FAIL: Clang :: CodeGenHLSL/ArrayAssignable.hlsl (1 of 1)
******************** TEST 'Clang :: CodeGenHLSL/ArrayAssignable.hlsl' FAILED ********************
Exit Code: 1
Command Output (stderr):
--
/workspace/llvm-project/build/bin/clang -cc1 -internal-isystem /workspace/llvm-project/build/lib/clang/22/include -nostdsysteminc -triple dxil-pc-shadermodel6.3-library -finclude-default-header -emit-llvm -disable-llvm-passes -o - /workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl | /workspace/llvm-project/build/bin/FileCheck /workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl # RUN: at line 1
+ /workspace/llvm-project/build/bin/clang -cc1 -internal-isystem /workspace/llvm-project/build/lib/clang/22/include -nostdsysteminc -triple dxil-pc-shadermodel6.3-library -finclude-default-header -emit-llvm -disable-llvm-passes -o - /workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl
+ /workspace/llvm-project/build/bin/FileCheck /workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl
/workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl:144:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: call void @llvm.memcpy.p0.p2.i32(ptr align 4 [[C]], ptr addrspace(2) align 4 @c1, i32 8, i1 false)
^
<stdin>:149:108: note: scanning from here
call void @llvm.memcpy.p0.p0.i32(ptr align 4 %C, ptr align 4 @__const._Z11arr_assign8v.C, i32 8, i1 false)
^
...
Input file: <stdin>
Check file: /workspace/llvm-project/clang/test/CodeGenHLSL/ArrayAssignable.hlsl
-dump-input=help explains the following input dump.
Input was:
<<<<<<
.
.
.
144:
145: ; Function Attrs: alwaysinline convergent mustprogress norecurse nounwind
146: define hidden void @_Z11arr_assign8v() #2 {
147: entry:
148: %C = alloca [2 x float], align 4
149: call void @llvm.memcpy.p0.p0.i32(ptr align 4 %C, ptr align 4 @__const._Z11arr_assign8v.C, i32 8, i1 false)
next:144'0 X error: no match found
next:144'1 with "C" equal to "%C"
150: %0 = getelementptr inbounds [2 x float], ptr %C, i32 0
next:144'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
151: %load = load float, ptr addrspace(2) @c1, align 4
next:144'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
152: store float %load, ptr %0, align 4
next:144'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
153: %1 = getelementptr inbounds [2 x float], ptr %C, i32 1
next:144'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
154: %load1 = load float, ptr addrspace(2) getelementptr inbounds (i8, ptr addrspace(2) @c1, i32 32), align 4
next:144'0 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
|
This shader fails to compile due to hitting an assert: // compile args: -T cs_6_2 -E CSMain
typedef uint32_t4 uint32_t8[2];
void Foo(uint32_t8) {}
cbuffer Constants {
uint32_t8 s;
}
[numthreads(1, 1, 1)]
void CSMain() {
Foo(s);
}Removing the typedef and just using |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should demonstrate #147352 well enough to look at how it will affect the backends, but it still needs a fair amount of work and cleanup.
i8for the padding at this point, rather thandx.Padding/spv.Paddingtypes. This is fairly easy to change and will come shortly