Correctly wrap/unwrap DebugLocalVariable ArgNumber in OpConstant

[AdamBrouwersHarries]

The NonSemantic.Shader.DebugInfo spec requires that the (optional) ArgNumber operand for a DebugLocalVariable operation is the id of a 32-bit OpConstant.

This patch ensures that both the LLVM to SPIR-V and SPIR-V to LLVM paths through the translator respect this requirement.

[CLAassistant]

All committers have signed the CLA.

[AdamBrouwersHarries]

Note, this addresses #3768

[MrSidims]

The patch makes sense to me, but I'm slightly surprised, that no test changes are required. If so - it means, that we need a dedicated test for the fix.

Regardless, would be nice to have review from @vmaksimo and @svenvh

[AdamBrouwersHarries]

The patch makes sense to me, but I'm slightly surprised, that no test changes are required. If so - it means, that we need a dedicated test for the fix.

There do need to be some test changes - that's why CI has been failing. I haven't had capacity to work on it yet this week.

Testing is a little tricky. The SPIR-V -> LLVM case is obvious enough, but requires a massive kernel with 65k+ IDs. LLVM to SPIR-V is less obvious, as LLVM inherently can't produce an ArgNumber that will overflow its own storage. I've encountered this issue "in the wild" in SPIR-V generated by Intel's SYCL compiler, so perhaps I should look into how that does it in more detail.

[MrSidims]

I suspect, that the tests are failing not because of this patch:

+ /home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/build/bin/FileCheck /home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/llvm-project/llvm/projects/SPIRV-LLVM-Translator/test/transcoding/vector_casts.ll
/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/llvm-project/llvm/projects/SPIRV-LLVM-Translator/test/transcoding/vector_casts.ll:18:10: error: CHECK: expected string not found in input
; CHECK: call spir_func <8 x i16> @_Z14convert_short8Dv8_c(<8 x i8>
         ^
<stdin>:1:1: note: scanning from here
; ModuleID = '/home/runner/work/SPIRV-LLVM-Translator/SPIRV-LLVM-Translator/build/projects/SPIRV-LLVM-Translator/test/test_output/transcoding/Output/vector_casts.ll.tmp.llc.rev.bc'
^
<stdin>:16:7: note: possible intended match here
 %1 = call spir_func <8 x i16> @_Z14convert_short8Dv8_h(<8 x i8> %in)
      ^

The SPIR-V -> LLVM case is obvious enough, but requires a massive kernel with 65k+ IDs.

Make sense. I won't push for extra tests then :) Still requires approval from the maintainers.

I'm closing/re-opening the PR to force new HEAD for the tests.

[MrSidims]

# error: command failed with exit status: -11

--

********************
Testing: 0  2  4  6  8  10 12 14 16 18 20 22 24 26 28 30 32 34 36 38 40 42 44 46 48 50 52 54 56 58 60 62 64 66 68 70 72 74 76 78 80 82 84 86 88 90 92 94 96 98 
********************
Failed Tests (1):
  LLVM_SPIRV :: DebugInfo/NonSemantic/basic-type-with-flag.spt
  

okay, this is something new

[AdamBrouwersHarries]

I suspect, that the tests are failing not because of this patch:

The test DebugInfo/NonSemantic/basic-type-with-flag.spt should definitely be failing because of this patch - I need to spend some time rewriting it to use an OpConstant properly.

[AdamBrouwersHarries]

Ah, I've realised that the test was actually written by @MrSidims, which is handy. For some reason, I can't actually disassemble the spir-v using spirv tools, and the spt file is not particularly editable. Do you happen to remember what the original source code which generated the spir-v looked like?

[AdamBrouwersHarries]

# error: command failed with exit status: -11

--

********************
Testing: 0  2  4  6  8  10 12 14 16 18 20 22 24 26 28 30 32 34 36 38 40 42 44 46 48 50 52 54 56 58 60 62 64 66 68 70 72 74 76 78 80 82 84 86 88 90 92 94 96 98 
********************
Failed Tests (1):
  LLVM_SPIRV :: DebugInfo/NonSemantic/basic-type-with-flag.spt
  

okay, this is something new

Yes - sorry, I was slow to respond. That test doesn't correctly wrap the ArgNumber in an OpConstant, so will fail with the new patch.

[MrSidims]

Do you happen to remember what the original source code which generated the spir-v looked like?

Nope. I could search at Intel's issue tracking system, but I've lost access to it some time ago :) May be @vmaksimo can help.

You can use llvm-spirv -to-binary to convert .spt to .spv and then use SPIR-V tools to disassemble.
UPD: ah, probably is that what you have tried already.

Alternatively, just convert it back to LLVM IR and it would give a ground to play with IR.

[AdamBrouwersHarries]

Do you happen to remember what the original source code which generated the spir-v looked like?

Nope. I could search at Intel's issue tracking system, but I've lost access to it some time ago :) May be @vmaksimo can help.

You can use llvm-spirv -to-binary to convert .spt to .spv and then use SPIR-V tools to disassemble.

Alternatively, just convert it back to LLVM IR and it would give a ground to play with IR.

Unfortunately the resulting .spv fails disassembly, with "error: 43: Invalid storage class operand: 4294967295". Converting from llvm IR -> SPIR-V -> Disassembly also fails, which is odd.

EDIT: Actually, that might have been the error from the LLVM -> SPIR-V -> SPIR-V ASM. Going straight from SPIR-V to SPIR-V ASM with spirv-dis gives:

error: 9: Invalid instruction OpString starting at word 40: expected no more operands after 4 words, but stated word count is 8.

I think it might be easier to try and re-create the SPIR-V from a fresh test with the same behaviour.

[MrSidims]

Apologies, I really have no clue, what did I mean here :D

d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  1) ;; This test checks, that SPIR-V module generated by another SPIR-V producer
d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  2) ;; containing FlagUnknownPhysicalLayout flag of DebugTypeBasic can be
d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  3) ;; consumed without issues

[AdamBrouwersHarries]

Apologies, I really have no clue, what did I mean here :D

d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  1) ;; This test checks, that SPIR-V module generated by another SPIR-V producer
d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  2) ;; containing FlagUnknownPhysicalLayout flag of DebugTypeBasic can be
d43a4e852 (Dmitry Sidorov               2023-04-26 12:20:30 +0200  3) ;; consumed without issues

No problem! I seem to have a knack for finding random tests and code that people have forgotten about 😅

I'll see how far I can get recreating the llvm IR with an OpenCL kernel, and then use that to generate a new SPIR-V test.