Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DebugInfo] Fix instruction enumeration for template parameter #2314

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[DebugInfo] Fix instruction enumeration for template parameter #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions lib/SPIRV/SPIRVToLLVMDbgTran.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,23 @@ SPIRVToLLVMDbgTran::transTypeTemplateParameter(const SPIRVExtInst *DebugInst) {
false);
}

// Workaround for the incorrect enum used for OpTypeTemplateTemplateParameter
// and OpTypeTemplateParameterPack instructions previously. The W/A added
// to enforce compatibility with previously generated SPIR-V modules.
DINode *SPIRVToLLVMDbgTran::transTypeTemplateParameterInst(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only tested locally. Ideally we should add an infrastructure to test pre-compiled binaries to ensure backward compatibility (that was probably also discussed during tooling wg call). I'll open an issue in github for this.

const SPIRVExtInst *DebugInst) {
constexpr SPIRVWord IndexToCheck =
SPIRVDebug::Operand::TypeTemplateTemplateParameter::TemplateNameIdx;
const SPIRVWordVec &Ops = DebugInst->getArguments();
// We can't distinguish these 2 instructions by number of SPIR-V words which
// would be better. But we know, that the second parameter of
// OpTypeTemplateTemplateParameter is 'Template Name', which for
// OpTypeTemplateParameterPack it is 'Source'.
if (BM->get<SPIRVValue>(Ops[IndexToCheck])->getOpCode() == OpString)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, we have a workaround like this locally. However, I was worried that it could be DebugInfoNone (I don't find the spec very clear about which operands are allowed to be DebugInfoNone)

I came up with a multi-stage guesstimation firstly using the number of operands, which may be more than 10 for only DebugTypeTemplateParameterPack, or failing that this operand being OpString for DebugTypeTemplateTemplateParameter, or this operand being DebugSource for DebugTypeTemplateParameterPack or if both of those are DebugInfoNone then some tie breaker I've forgotten about. It was all a bit complicated.

If this can only truly ever be OpString and never DebugInfoNone then this looks fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed

return transTypeTemplateTemplateParameter(DebugInst);
return transTypeTemplateParameterPack(DebugInst);
}

DINode *SPIRVToLLVMDbgTran::transTypeTemplateTemplateParameter(
const SPIRVExtInst *DebugInst) {
using namespace SPIRVDebug::Operand::TypeTemplateTemplateParameter;
Expand Down Expand Up @@ -1493,10 +1510,8 @@ MDNode *SPIRVToLLVMDbgTran::transDebugInstImpl(const SPIRVExtInst *DebugInst) {
return transTypeTemplateParameter(DebugInst);

case SPIRVDebug::TypeTemplateTemplateParameter:
return transTypeTemplateTemplateParameter(DebugInst);

case SPIRVDebug::TypeTemplateParameterPack:
return transTypeTemplateParameterPack(DebugInst);
return transTypeTemplateParameterInst(DebugInst);

case SPIRVDebug::TypeTemplate:
return transTypeTemplate(DebugInst);
Expand Down
1 change: 1 addition & 0 deletions lib/SPIRV/SPIRVToLLVMDbgTran.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ class SPIRVToLLVMDbgTran {
DINode *transTypeTemplateParameter(const SPIRVExtInst *DebugInst);
DINode *transTypeTemplateTemplateParameter(const SPIRVExtInst *DebugInst);
DINode *transTypeTemplateParameterPack(const SPIRVExtInst *DebugInst);
DINode *transTypeTemplateParameterInst(const SPIRVExtInst *DebugInst);

MDNode *transTypeTemplate(const SPIRVExtInst *DebugInst);

Expand Down
4 changes: 2 additions & 2 deletions lib/SPIRV/libSPIRV/SPIRV.debug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ enum Instruction {
TypePtrToMember = 13,
TypeTemplate = 14,
TypeTemplateParameter = 15,
TypeTemplateParameterPack = 16,
TypeTemplateTemplateParameter = 17,
TypeTemplateTemplateParameter = 16,
TypeTemplateParameterPack = 17,
GlobalVariable = 18,
FunctionDeclaration = 19,
Function = 20,
Expand Down
49 changes: 49 additions & 0 deletions test/DebugInfo/IncorrectInstructionEnumerationSPIRVtoLLVM.spvasm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
; This test checks that the decoding of DebugTypeTemplateParameterPack and
; DebugTypeTemplateTemplateParameter is correct.

; REQUIRES: spirv-as

; RUN: spirv-as --target-env spv1.3 %s -o %t.spv
; RUN: llvm-spirv -r %t.spv -o - | llvm-dis | FileCheck %s

; SPIR-V
; Version: 1.1
; Generator: Khronos LLVM/SPIR-V Translator; 14
; Bound: 16
; Schema: 0
OpCapability Addresses
OpCapability Kernel
%1 = OpExtInstImport "OpenCL.std"
%2 = OpExtInstImport "OpenCL.DebugInfo.100"
OpMemoryModel Physical64 OpenCL
OpEntryPoint Kernel %5 "func"
%8 = OpString "/tmp/test.cpp"
%9 = OpString "//__CSK_MD5:18aa9ce738eaafc7b7b7181c19092815"
%12 = OpString "func"
%14 = OpString ""
%15 = OpString "T"
%16 = OpString "U"
%17 = OpString "Foo"
OpSource OpenCL_CPP 100000
OpName %entry "entry"
%void = OpTypeVoid
%4 = OpTypeFunction %void
%10 = OpExtInst %void %2 DebugSource %8 %9
%11 = OpExtInst %void %2 DebugCompilationUnit 65536 5 %10 OpenCL_CPP
%13 = OpExtInst %void %2 DebugInfoNone
%20 = OpExtInst %void %2 DebugFunction %12 %13 %10 1 0 %11 %14 FlagIsDefinition|FlagPrototyped|FlagIsOptimized 2 %5 %13
%18 = OpExtInst %void %2 DebugTypeTemplateParameterPack %15 %10 0 0 %13 %13
%19 = OpExtInst %void %2 DebugTypeTemplateTemplateParameter %16 %17 %10 0 0
%21 = OpExtInst %void %2 DebugTypeTemplate %20 %18 %19
%5 = OpFunction %void None %4
%entry = OpLabel
OpReturn
OpFunctionEnd

; CHECK: = distinct !DISubprogram(name: "func",
; CHECK-SAME: templateParams: [[TPARAMS:![0-9]+]]{{[,)]}}

; CHECK-DAG: [[TPARAMS]] = !{[[PACKPARAM:![0-9]+]], [[TTPARAM:![0-9]+]]}
; CHECK-DAG: [[PACKPARAM]] = !DITemplateValueParameter(tag: DW_TAG_GNU_template_parameter_pack, name: "T", value: [[PACK:![0-9]+]])
; CHECK-DAG: [[PACK]] = !{null, null}
; CHECK-DAG: [[TTPARAM]] = !DITemplateValueParameter(tag: DW_TAG_GNU_template_template_param, name: "U", value: !"Foo")
Loading