From 48338ac0e33670d63aa2fa42484b5acf1c228cbc Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Fri, 29 May 2020 18:04:51 -0400 Subject: [PATCH 1/4] [spirv] fix DebugTypeArray bug The pseudo code for lowering an array type in DebugTypeVisitor is ``` elementType = arrayType->element if elementType is an array type (DebugTypeArray for elementType).appendOneMoreDimensionAtTheEnd(count) else new DebugTypeArray(lower(arrayType->element), count) ``` When it appends one more dimension at the end of previously created DebugTypeArray, it reuses the existing DebugTypeArray object, which result in a wrong DebugTypeArray for a previously shown array type. For example, ``` float foo[2]; --> dbg_type_array = DebugTypeArray(float, 2) float bar[2][3]; --> dbg_type_array.appendOneMoreDimensionAtTheEnd(3) // When emitting the SPIRV, dbg_type_array is // DebugTypeArray(float, 2, 3) which is incorrect for "foo" ``` This commit lets DebugTypeVisitor create a new DebugTypeArray object for new array type to fix this bug. --- .../include/clang/SPIRV/SpirvInstruction.h | 2 +- tools/clang/lib/SPIRV/DebugTypeVisitor.cpp | 19 +++++++------- tools/clang/lib/SPIRV/EmitVisitor.cpp | 9 ++++--- .../rich.debug.type.array-from-same-type.hlsl | 25 +++++++++++++++++++ .../unittests/SPIRV/CodeGenSpirvTest.cpp | 4 +++ 5 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl diff --git a/tools/clang/include/clang/SPIRV/SpirvInstruction.h b/tools/clang/include/clang/SPIRV/SpirvInstruction.h index 8fcd5e2361..0578e5f459 100644 --- a/tools/clang/include/clang/SPIRV/SpirvInstruction.h +++ b/tools/clang/include/clang/SPIRV/SpirvInstruction.h @@ -2245,7 +2245,7 @@ class SpirvDebugTypeArray : public SpirvDebugType { bool invokeVisitor(Visitor *v) override; SpirvDebugType *getElementType() const { return elementType; } - llvm::SmallVector &getElementCount() { return elementCount; } + llvm::SmallVector getElementCount() { return elementCount; } uint32_t getSizeInBits() const override { // TODO: avoid integer overflow diff --git a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp index 46d967bd73..8046d4367c 100644 --- a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp +++ b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp @@ -262,16 +262,15 @@ DebugTypeVisitor::lowerToDebugType(const SpirvType *spirvType) { auto *arrType = dyn_cast(spirvType); SpirvDebugInstruction *elemDebugType = lowerToDebugType(arrType->getElementType()); + + llvm::SmallVector counts; if (auto *dbgArrType = dyn_cast(elemDebugType)) { - auto &counts = dbgArrType->getElementCount(); - // Note that this is reverse order of dimension. We must iterate the - // count array in a reverse order when we actually emit it. - counts.push_back(arrType->getElementCount()); - debugType = dbgArrType; - } else { - debugType = spvContext.getDebugTypeArray(spirvType, elemDebugType, - {arrType->getElementCount()}); + counts = dbgArrType->getElementCount(); + elemDebugType = dbgArrType->getElementType(); } + counts.push_back(arrType->getElementCount()); + + debugType = spvContext.getDebugTypeArray(spirvType, elemDebugType, counts); break; } case SpirvType::TK_Vector: { @@ -291,7 +290,9 @@ DebugTypeVisitor::lowerToDebugType(const SpirvType *spirvType) { SpirvDebugInstruction *elemDebugType = lowerToDebugType(matType->getElementType()); debugType = spvContext.getDebugTypeArray( - spirvType, elemDebugType, {matType->numRows(), matType->numCols()}); + spirvType, elemDebugType, + llvm::SmallVector( + {matType->numRows(), matType->numCols()})); break; } case SpirvType::TK_Pointer: { diff --git a/tools/clang/lib/SPIRV/EmitVisitor.cpp b/tools/clang/lib/SPIRV/EmitVisitor.cpp index 0618df71ed..58c850d0ba 100644 --- a/tools/clang/lib/SPIRV/EmitVisitor.cpp +++ b/tools/clang/lib/SPIRV/EmitVisitor.cpp @@ -1343,13 +1343,16 @@ bool EmitVisitor::visit(SpirvDebugTypeArray *inst) { curInst.push_back(inst->getDebugOpcode()); curInst.push_back( getOrAssignResultId(inst->getElementType())); - for (auto it = inst->getElementCount().rbegin(); - it != inst->getElementCount().rend(); ++it) { + + // This is a reverse order of dimensions, thereby emitting in a reverse order. + llvm::SmallVector counts = inst->getElementCount(); + for (int i = counts.size() - 1; i >= 0; --i) { const auto countId = typeHandler.getOrCreateConstantInt( - llvm::APInt(32, *it), context.getUIntType(32), + llvm::APInt(32, counts[i]), context.getUIntType(32), /* isSpecConst */ false); curInst.push_back(countId); } + finalizeInstruction(&richDebugInfo); return true; } diff --git a/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl new file mode 100644 index 0000000000..12c328d8f4 --- /dev/null +++ b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl @@ -0,0 +1,25 @@ +// Run: %dxc -T ps_6_0 -E main -fspv-debug=rich + +struct UBO +{ + float4x4 a; + float4x4 b[3]; + float4 c; +}; + +cbuffer ubo : register(b0) { UBO ubo; } + +// CHECK: [[fooName:%\d+]] = OpString "foo" +// CHECK: [[aName:%\d+]] = OpString "a" +// CHECK: [[bName:%\d+]] = OpString "b" + +// CHECK: [[matf4v4:%\d+]] = OpExtInst %void [[ext:%\d+]] DebugTypeArray [[dbg_f:%\d+]] %uint_4 %uint_4 +// CHECK: [[matf4v4_arr3:%\d+]] = OpExtInst %void [[ext]] DebugTypeArray [[dbg_f]] %uint_3 %uint_4 %uint_4 +// CHECK: OpExtInst %void [[ext]] DebugLocalVariable [[fooName]] [[matf4v4]] +// CHECK: OpExtInst %void [[ext]] DebugTypeMember [[aName]] [[matf4v4]] +// CHECK: OpExtInst %void [[ext]] DebugTypeMember [[bName]] [[matf4v4_arr3]] + +void main() { + float4x4 foo = ubo.a; + foo += ubo.b[0] * ubo.c[0]; +} diff --git a/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp b/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp index 4b199ae71d..8dab1b4551 100644 --- a/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp +++ b/tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp @@ -2317,6 +2317,10 @@ TEST_F(FileTest, RichDebugInfoTypeArray) { runFileTest("rich.debug.type.array.hlsl", Expect::Success, /*runValidation*/ runValidationForRichDebugInfo); } +TEST_F(FileTest, RichDebugInfoTypeArrayFromSameType) { + runFileTest("rich.debug.type.array-from-same-type.hlsl", Expect::Success, + /*runValidation*/ runValidationForRichDebugInfo); +} TEST_F(FileTest, RichDebugInfoTypeFunction) { runFileTest("rich.debug.type.function.hlsl", Expect::Success, /*runValidation*/ runValidationForRichDebugInfo); From 890dbbe6466384a451a4be846a75e180123435d5 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Wed, 3 Jun 2020 12:01:42 -0400 Subject: [PATCH 2/4] Fix test failure --- .../rich.debug.type.array-from-same-type.hlsl | 12 ++++++------ .../test/CodeGenSPIRV/rich.debug.type.array.hlsl | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl index 12c328d8f4..0184109a79 100644 --- a/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl +++ b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array-from-same-type.hlsl @@ -9,15 +9,15 @@ struct UBO cbuffer ubo : register(b0) { UBO ubo; } -// CHECK: [[fooName:%\d+]] = OpString "foo" -// CHECK: [[aName:%\d+]] = OpString "a" // CHECK: [[bName:%\d+]] = OpString "b" +// CHECK: [[aName:%\d+]] = OpString "a" +// CHECK: [[fooName:%\d+]] = OpString "foo" -// CHECK: [[matf4v4:%\d+]] = OpExtInst %void [[ext:%\d+]] DebugTypeArray [[dbg_f:%\d+]] %uint_4 %uint_4 -// CHECK: [[matf4v4_arr3:%\d+]] = OpExtInst %void [[ext]] DebugTypeArray [[dbg_f]] %uint_3 %uint_4 %uint_4 -// CHECK: OpExtInst %void [[ext]] DebugLocalVariable [[fooName]] [[matf4v4]] -// CHECK: OpExtInst %void [[ext]] DebugTypeMember [[aName]] [[matf4v4]] +// CHECK: [[matf4v4_arr3:%\d+]] = OpExtInst %void [[ext:%\d+]] DebugTypeArray [[dbg_f:%\d+]] %uint_3 %uint_4 %uint_4 // CHECK: OpExtInst %void [[ext]] DebugTypeMember [[bName]] [[matf4v4_arr3]] +// CHECK: [[matf4v4:%\d+]] = OpExtInst %void [[ext]] DebugTypeArray [[dbg_f]] %uint_4 %uint_4 +// CHECK: OpExtInst %void [[ext]] DebugTypeMember [[aName]] [[matf4v4]] +// CHECK: OpExtInst %void [[ext]] DebugLocalVariable [[fooName]] [[matf4v4]] void main() { float4x4 foo = ubo.a; diff --git a/tools/clang/test/CodeGenSPIRV/rich.debug.type.array.hlsl b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array.hlsl index fe146cbe01..255d392740 100644 --- a/tools/clang/test/CodeGenSPIRV/rich.debug.type.array.hlsl +++ b/tools/clang/test/CodeGenSPIRV/rich.debug.type.array.hlsl @@ -12,10 +12,10 @@ // CHECK: [[bool:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic [[boolName]] %uint_32 Boolean // CHECK: [[int:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic [[intName]] %uint_32 Signed // CHECK: [[uint:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic [[uintName]] %uint_32 Unsigned +// CHECK: [[float:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic [[floatName]] %uint_32 Float // CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugTypeArray [[S]] %uint_8 // CHECK: [[boolv4:%\d+]] = OpExtInst %void [[set]] DebugTypeVector [[bool]] 4 // CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugTypeArray [[boolv4]] %uint_7 -// CHECK: [[float:%\d+]] = OpExtInst %void [[set]] DebugTypeBasic [[floatName]] %uint_32 Float // CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugTypeArray [[float]] %uint_8 %uint_4 // CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugTypeArray [[int]] %uint_8 // CHECK: {{%\d+}} = OpExtInst %void [[set]] DebugTypeArray [[uint]] %uint_4 From 631444cc30e7fb9cf74c94c0a5ebc73c072d23b4 Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Fri, 5 Jun 2020 11:39:22 -0400 Subject: [PATCH 3/4] Update based on code review --- tools/clang/lib/SPIRV/DebugTypeVisitor.cpp | 4 +--- tools/clang/lib/SPIRV/EmitVisitor.cpp | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp index 8046d4367c..6245d9c52f 100644 --- a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp +++ b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp @@ -290,9 +290,7 @@ DebugTypeVisitor::lowerToDebugType(const SpirvType *spirvType) { SpirvDebugInstruction *elemDebugType = lowerToDebugType(matType->getElementType()); debugType = spvContext.getDebugTypeArray( - spirvType, elemDebugType, - llvm::SmallVector( - {matType->numRows(), matType->numCols()})); + spirvType, elemDebugType, {matType->numRows(), matType->numCols()}); break; } case SpirvType::TK_Pointer: { diff --git a/tools/clang/lib/SPIRV/EmitVisitor.cpp b/tools/clang/lib/SPIRV/EmitVisitor.cpp index 58c850d0ba..78a1d82b1a 100644 --- a/tools/clang/lib/SPIRV/EmitVisitor.cpp +++ b/tools/clang/lib/SPIRV/EmitVisitor.cpp @@ -1346,9 +1346,9 @@ bool EmitVisitor::visit(SpirvDebugTypeArray *inst) { // This is a reverse order of dimensions, thereby emitting in a reverse order. llvm::SmallVector counts = inst->getElementCount(); - for (int i = counts.size() - 1; i >= 0; --i) { + for (auto it = counts.rbegin(); it != counts.rend(); ++it) { const auto countId = typeHandler.getOrCreateConstantInt( - llvm::APInt(32, counts[i]), context.getUIntType(32), + llvm::APInt(32, *it), context.getUIntType(32), /* isSpecConst */ false); curInst.push_back(countId); } From 84efc2d9420a5ff03964e94403d06e1c166bb24f Mon Sep 17 00:00:00 2001 From: Jaebaek Seo Date: Fri, 5 Jun 2020 12:03:07 -0400 Subject: [PATCH 4/4] This might be more efficient --- tools/clang/include/clang/SPIRV/SpirvInstruction.h | 2 +- tools/clang/lib/SPIRV/DebugTypeVisitor.cpp | 3 ++- tools/clang/lib/SPIRV/EmitVisitor.cpp | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/clang/include/clang/SPIRV/SpirvInstruction.h b/tools/clang/include/clang/SPIRV/SpirvInstruction.h index 0578e5f459..8fcd5e2361 100644 --- a/tools/clang/include/clang/SPIRV/SpirvInstruction.h +++ b/tools/clang/include/clang/SPIRV/SpirvInstruction.h @@ -2245,7 +2245,7 @@ class SpirvDebugTypeArray : public SpirvDebugType { bool invokeVisitor(Visitor *v) override; SpirvDebugType *getElementType() const { return elementType; } - llvm::SmallVector getElementCount() { return elementCount; } + llvm::SmallVector &getElementCount() { return elementCount; } uint32_t getSizeInBits() const override { // TODO: avoid integer overflow diff --git a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp index 6245d9c52f..d8b9bd6cc9 100644 --- a/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp +++ b/tools/clang/lib/SPIRV/DebugTypeVisitor.cpp @@ -265,7 +265,8 @@ DebugTypeVisitor::lowerToDebugType(const SpirvType *spirvType) { llvm::SmallVector counts; if (auto *dbgArrType = dyn_cast(elemDebugType)) { - counts = dbgArrType->getElementCount(); + counts.insert(counts.end(), dbgArrType->getElementCount().begin(), + dbgArrType->getElementCount().end()); elemDebugType = dbgArrType->getElementType(); } counts.push_back(arrType->getElementCount()); diff --git a/tools/clang/lib/SPIRV/EmitVisitor.cpp b/tools/clang/lib/SPIRV/EmitVisitor.cpp index 78a1d82b1a..a824c17560 100644 --- a/tools/clang/lib/SPIRV/EmitVisitor.cpp +++ b/tools/clang/lib/SPIRV/EmitVisitor.cpp @@ -1345,8 +1345,8 @@ bool EmitVisitor::visit(SpirvDebugTypeArray *inst) { getOrAssignResultId(inst->getElementType())); // This is a reverse order of dimensions, thereby emitting in a reverse order. - llvm::SmallVector counts = inst->getElementCount(); - for (auto it = counts.rbegin(); it != counts.rend(); ++it) { + for (auto it = inst->getElementCount().rbegin(); + it != inst->getElementCount().rend(); ++it) { const auto countId = typeHandler.getOrCreateConstantInt( llvm::APInt(32, *it), context.getUIntType(32), /* isSpecConst */ false);