dedup BuiltIn OpVariable

[Firestar99]

• Extracted from spirv-std functions to query builtins (fluffy's fork) #535 by @fluffysquirrels
• deduplicate Input OpVaraibles with BuiltIn decoration
• caused by builtins declared by asm! blocks, see asm fn_ptr support #534
  • can't happen with current entry point system

[Firestar99]

This is nothing against you @fluffysquirrels or your code, it's perfectly good code you wrote here.

<rant>
But every time I look at rspirv's module and how we're running our post-link passes on there, I'm disgusted by it. Like why do we have to go through all global declarations and filter out all the OpVariable instructions? With the amount of processing we're doing on them, we should really just have a separate Vec<OpVariable>. Or even better, for all builtins just a HashMap<Builtin, OpVariable> and dedup them on insertion / during linking. I so want to get rid of rspirv' Module and replace it with a custom datastructure that's specialized on our access patterns, not some general purpose whatever. Really want to rewrite this part or port it to spirt or something. 😭
</rant>

[fluffysquirrels]

Ha! I see exactly what you mean, @Firestar99 and no offense taken :)

rspirv has its own version of this in its rspirv::sr ("structured representation") module, which I saw but soon realised I couldn't use currently. So instead I stuck to the existing style and scraped the data I needed back out of low-level spirv data.

I also think rust-gpu would be improved with some typed layer like rspirv::sr or Rust's MIR.

[Firestar99]

sadly rspirv's sr doesn't allow you to turn it back into proper instructions again, and there's several other things I would like to change about it...

... give me a week or two, may have something in the works already

[Firestar99]

Retracting for now:

• fragment shader builtins that are integers need to also be annotated with the Flat decoration, need to verify this handles it correctly. I need to research where this is spec'ed, just seen spirv-val complain about it.
• Certain shader builtins allow redeclaration to specify the array size of them. Those still need to be merged, but only of the type is the same. I moved this new pass runs before type deduplication to not have to run another fixup pass here that type dedup already does... so may have to split up type dedup.
• we may need this to also support OpVariable _ Output, though may move this to a followup PR
  • Output needs to respect PerPrimitive decoration as a separate dedup class

[fluffysquirrels]

If the problems are all with non-compute stuff, why not just limit this to compute-only builtins? Could use an allow list and expand it when ready.

I considered output variables also, but decided to avoid them for now since I had no examples to test.

[Firestar99]

I don't think it's too much more work to get input builtins up and running. I've actually rewrote your #535 in spirv-std-builtin-fn3, which is based on #540 to give it a clean structure. Has all the builtins for compute, subgroup, vertex and fragment. Not yet PRed.

As well as a first experiment in Output builtins: 87d115f

Having to research every single one takes time, but also revealed some wrong assumptions and these issues.

[fluffysquirrels]

Not sure it makes sense to use this pattern for output builtins, e.g. for vertex shaders. It will make it very difficult to see what the actual signature (in and out parameters) of the shaders are.

The great thing about the compute shader input builtins is that they were all always available automatically, so this did not add any confusion.

Whereas (from my limited knowledge) in a vertex or fragment shader you actually do want to see what input it is relying upon and where the output is being written.