KhronosGroup / SPIRV-Tools

Apache License 2.0
1.07k stars 551 forks source link

Generator comment in spirv-dis output is failing to look up name from spir-v.xml #5717

Closed andfau-amd closed 2 months ago

andfau-amd commented 3 months ago

spirv-dis on matmul.spv.zip outputs text beginning like this for me:

; SPIR-V
; Version: 1.0
; Generator: Khronos; 22
; Bound: 75
; Schema: 0

The Generator: line is very strange. Why doesn't it know what the generator is? 22 is a registered ID:

https://github.com/KhronosGroup/SPIRV-Headers/blob/2acb319af38d43be3ea76bfabf3998e5281d8d12/include/spirv/spir-v.xml#L75

I wondered if maybe SPIRV-Tools wasn't actually generating its lookup table from the xml, but no, I found the generated file in the build:

$ find . -name generators.inc
./build/generators.inc

And it does contain the relevant line:

{22, "Google", "MLIR SPIR-V Serializer", "Google MLIR SPIR-V Serializer"},

So I'm not sure what's going on. Code bug? Build system issue?

Reproduction steps

I'm using Ubuntu 22.04 LTS in WSL2 on Windows 11, if that helps.

$ git clone https://github.com/KhronosGroup/SPIRV-Tools.git
$ cd SPIRV-Tools/
$ git clone https://github.com/KhronosGroup/SPIRV-Headers.git external/spirv-headers
$ cmake -B build -S . -G Ninja
$ cmake --build build
$ build/tools/spirv-dis matmul.spv
andfau-amd commented 3 months ago

Found the bug:

diff --git a/source/disassemble.cpp b/source/disassemble.cpp
index aa20fab6..c9719062 100644
--- a/source/disassemble.cpp
+++ b/source/disassemble.cpp
@@ -650,7 +650,7 @@ void InstructionDisassembler::EmitHeaderVersion(uint32_t version) {

 void InstructionDisassembler::EmitHeaderGenerator(uint32_t generator) {
   const char* generator_tool =
-      spvGeneratorStr(SPV_GENERATOR_TOOL_PART(generator));
+      spvGeneratorStr(generator);
   stream_ << "; Generator: " << generator_tool;
   // For unknown tools, print the numeric tool value.
   if (0 == strcmp("Unknown", generator_tool)) {

I won't submit a patch for this. If anyone else is interested, please steal it. It's low-hanging fruit.

andfau-amd commented 3 months ago

Huh, this code seems like it hasn't changed in eight years, was this always broken or am I missing something? I could have sworn this used to work.

s-perron commented 2 months ago

Are you sure that the tool generated the "generator" word correctly? Part of the word is in two part. One part is the id for the tool, and the other is a version number. This looks like the spir-v has 0 for the generator, and 22 for the version. That is the way it is being dumped by spirv-dis.

When I test using thing generated by DXC, it works correctly.

s-perron commented 2 months ago

I just did a hexdump of the spirv file you provided. The generator word is "16 00 00 00". This should be "00 00 16 00" Spirv generated by DXC has "00 00 0E 00".

https://github.com/KhronosGroup/SPIRV-Headers/blob/3c355ec439dcf821c50fb4660ef0e50d19ae2b63/include/spirv/spir-v.xml#L35-L39

andfau-amd commented 2 months ago

I see, thanks for correcting my mistake.