@dnovillo thanks for sharing this. It is super insightful. I’m new to debug info and SPIR-V so this has helped.
In summary, your idea sounds great to me if we can make it work. My goal is to eventually support NonSemantic.Shader.DebugInfo.100 and also make sure that it works well with the SPIRV-LLVM translator. Hopefully, here are some answers to the questions plus some new questions from my side.
Is there a technical reason the MachineFunctionPass model has to be kept for #183117 and #183122, rather than converting SPIRVEmitNonSemanticDI to an AsmPrinterHandler subclass first?
Regarding #183117 and #183122, I wasn’t aware of AsmPrinterHandler. Your plan sounds great to me if it can fit in the current backend’s implementation.
The PRs kept the MachineFunctionPass model because my understanding is that the BE has no notion of SPIR-V module until the end. For example, global variable are affected in a similar way, meaning that each function declares its own copy until the SPIR-V module is materialized and deduplication is applied. I was planning to do similarly for DebugCompilationUnit and DebugFunction. However, I like your plan because this may be avoided (and also for other opcodes).
I’m unsure whether it is safe in the context of SPIR-V BE to refer to registers across different function definitions. The AsmPrinterHandler approach may not face this problem because it is triggered at the time that the SPIR-V module is materialized.
I’m trying to picture possible challenges based on my SPIR-V/BE understanding. I think you may have already considered this but I’m raising this up just to be safe.
The main challenge that I see is that some instructions must be emitted in specific sections. If I follow, the AsmPrinterHandler approach must be aware of it. In other words, the AsmPrinterHandler must make sure that when emitting an instruction all its required operands, if they belong to a different section, they are already emitted (and keep track of their registers), right?
Could this approach still have the risk of emitting duplicate instructions? If so, can it be mitigated?
Is there someone working on DebugLine / DebugNoLine (opcodes 103, 104)?
Last time I asked in the LLVM SPIRV Backend meeting no one else was working on debug info. My original plan was to tackle those instructions once we had support for DISubProgram and DebugFunction, although I’d be more than happy to coordinate our efforts.
There is also #179975, a WIP PR covering the complete NSDI instruction set and a ModulePass conversion. @mgcarrasco, I understand that you are breaking it up into smaller pieces?
I tried to break that specific PR into smaller pieces but found too many issues at once and its size didn’t help either. In short, most of the its tests were not reversed translatable by the SPIRV-LLVM translator. I recall that at least it was somehow breaking the debug info support that is already in LLVM upstream.
I’ve been working on submitting smaller PRs on the same direction also to help in the reviewing process. #183117 and 183122 are my attempts so far. I wanted to gather feedback as soon as possible in case I was missing things like the ones you pointed out (thanks).
Concretely, I would replace SPIRVEmitNonSemanticDI by an AsmPrinterHandler subclass registered in SPIRVAsmPrinter, not extended as a pass.
Convert SPIRVEmitNonSemanticDI to an AsmPrinterHandler subclass registered in SPIRVAsmPrinter. This goes further than the ModulePass conversion in #179975 and aligns the NSDI writer structurally with DwarfDebug and CodeViewDebug.
This sounds good to me. If a PR is submitted just for the replacement/switch I’d be happy to review it. I can also relayer #183117 and #183122 and work on extra steps towards NSDI support within this new approach.
Land PR #183121 now. It is independent from my concerns.
If I follow, the NSDI writer approach would already emit the instructions in the appropriate sections and the handling in #183121 may no longer be required, right?
Questions:
- Just to learn, I noticed that you mentioned specializing the
AsmPrinterHandlerclass but not theDebugHandlerBaseone. Is there any reason for this?
Remarks:
- For us it is important to make sure that the emitted SPIR-V code can also be reversed translatable using the SPIRV-LLVM translator, although I know this somewhat orthogonal to the approach used to emit debug info in the BE.
Once again thanks for sharing this idea!
Best,
Manuel