deduplicate OpVariable

[khyperia]

I believe this may help getting things working on Android.

On an internal issue, @VZout said:

My minimal test case right now is just 2 entry points with vertex index to reproduce the pipeline creation failure

Here's my theory. If you have two shaders set up like this:

#[spirv(vertex)]
pub fn vs_1(#[spirv(vertex_index)] vert_id: Input<i32>) { }
#[spirv(vertex)]
pub fn vs_2(#[spirv(vertex_index)] vert_id: Input<i32>) { }

then right now, rust-gpu will generate two OpVariables: one per entry point. Each OpVariable will get used in their respective vertex shader, and all is well and good... until we arrive at buggy android shader compilers. My guess is that it emits some automatic intrinsic code, or something, idk, when it encounters a vertex index OpVariable. Then, it hits another vertex index OpVariable, and says "wait we already did that, oops, crash", even though they're only used in separate shaders, never at the same time.

My theory is that emitting a single OpVariable for both shaders will make it happy. But, I don't have @VZout's testing code, so I'm putting this up and seeing if it works~

However! An open question is what to deduplicate on. If the descriptor set, binding, location, type, or any other decoration differs, then obviously we shouldn't deduplicate it. But... what if it has a different OpName? For some use cases, we should keep both around! They're different names, different things, shows up differently in OpenGL when searching for uniforms by name! But for some use cases, we should deduplicate them - for example, the following would generate two vertex index OpVariables, which isn't great!

#[spirv(vertex)]
pub fn vs_1(#[spirv(vertex_index)] one: Input<i32>) { }
#[spirv(vertex)]
pub fn vs_2(#[spirv(vertex_index)] two: Input<i32>) { }

Right now, this code does consider name to be part of the unique key of an OpVariable - so it will not deduplicate the above, and keep two around.

---

Amusingly, I got this idea from a completely unrelated issue with a community member having issues with I think spirv-cross, which dumped out two identical variables in the source (because there's two OpVariables), which got sad. Context in the arkadia discord here.

[eddyb]

Might be simpler to start with a [0; 4] and write the bytes into it (with copy_from_slice IIRC)?

[VZout]

Great success! it renders. (Didn't read through the code)

[VZout]

Why did it merge.... @eddyb didn't aprove it.

[XAMPPRocky]

@VZout We only have 1 required reviewer, so if you request more than that, whoever approves first will cause it to be merged.

[eddyb]

Why did it merge.... @eddyb didn't aprove it.

FWIW I was fine with merging it, and the only code change I might've requested was #396 (comment) - but I expected more discussion might be needed on whether we should do this in the first place.

[khyperia]

@VZout We only have 1 required reviewer, so if you request more than that, whoever approves first will cause it to be merged.

Do note that we also have a requirement of 0 requested reviewers (and 0 requested changes), which means that 1 person approving will not cause it to be merged. But, @eddyb commented (like a reasonable person), and github says that clears the review request, so 0 requested reviews left, and merge.

FWIW I was fine with merging it, and the only code change I might've requested was #396 (comment) - but I expected more discussion might be needed on whether we should do this in the first place.

Yeah, was planning on doing that, as well as more discussion around OpName handling. Oh well I guess, can do that in a follow-up.