[DebugInfo] Fix instruction enumeration for template parameter #2314
Conversation
These two opcodes were mistakenly swapped when they were originally added, at least according to the DebugInfo, OpenCL.DebugInfo.100, and NonSemantic.Shader.DebugInfo.100 extended instruction sets. This might break existing third-party SPIR-V translators if they are accommodating this bug, mistakenly or otherwise. Original author: Fraser Cormack
|
@frasercrmck sorry for the delay and please take a look. The plan is to get approvals for this PR, afterwards I'll create backport PRs to other release branches, that have OpenCL DebugInfo implemented and after merge with some graceful period - I'll merge the PR on the main branch. (you may also incorporate changes from this PR into yours, so we can proceed with your PR, I don't mind anyway :) ) |
| // 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( |
There was a problem hiding this comment.
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.
| // 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) |
There was a problem hiding this comment.
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.
These two opcodes were mistakenly swapped when they were originally added, at least according to the DebugInfo, OpenCL.DebugInfo.100, and NonSemantic.Shader.DebugInfo.100 extended instruction sets.
This might break existing third-party SPIR-V translators if they are accommodating this bug, mistakenly or otherwise.
It's continuation of #2248
Original author: Fraser Cormack