Rust-GPU/rust-gpu

DCE for unused shared memory arguments

Opened this issue · 4 comments

When debugging code, I often end up commenting-out pieces of code, which sometimes results in DCE that leaves shared memory arguments unused.

Unfortunately this results in validation error:

error: error:0:0 - [VUID-StandaloneSpirv-None-10684] Invalid explicit layout decorations on type for operand '290[%_ptr_Workgroup__struct_106]'
         %scratch_space = OpVariable %_ptr_Workgroup__struct_106 Workgroup
  |
  = note: spirv-val failed
  = note: module `file.spv`

It'd be really nice from DX perspective to also DCE this unused workgroup argument so that it doesn't end up in SPIR-V file and passes validation as the result.

We have a linker pass called explicit_layout that duplicates the type layout declarations and removes the explicit layout from them. Primarily used for workgroup shared memory, which must explicitly not have an explicit layout since a few spirv-val versions, which seems to be failing here.
Afaik all params in the entry point are considered roots for DCE, so it will never remove them on it's own. It's usually spirv-opt that may do so. Though I wonder if it's worth implementing a DCE now, when #449 will likely refactor the entire entry point and require a new analysis for "used params" anyway.
(We should still see if we can fix the linker pass though)

eddyb commented

I ran into the OpEntryPoint list of globals recently, and for a while I was extremely confused (but the issue turned out to be an outdated set of live functions, being passed around, and accidentally "reviving" a dead function or something).

Because the solution was very simple (and entirely separate from that whole system), I didn't start poking at it, but I was considering adding some "used globals" tracking to spirt::spv::lift, so it can regenerate the list entirely on its own, and remove all hazards associated with it.

If anyone wants to test a quick fix (which I'm surprised I've forgotten I could "just" use):

// Seed the queues starting from the module exports.
for exportee in module.exports.values_mut() {
exportee
.inner_transform_with(&mut eraser)
.apply_to(exportee);
}

That whole loop can just be eraser.in_place_transform_module(module);.

And spirt already has the necessary logic to rebuild the exports if any key changes.

I'm turning your suggestion into a PR

Here's the PR: #456

It does indeed fix the validation issue, but it does not DCE the shared memory from the spirv output. @eddyb I assume we'd have to upgrade our DCE capabilities for that to happen? We can probably rely upon the driver doing it for us though. and I got DCE to work as well.