msl: handle the case of missing binding

[kvark]

The purpose of this code is to detect if a global variable can or can't be used legally by a given entry point. It assumes the resource binding is present and just checks if the override is set. I find it also useful to check if there is a binding at all. Hence the new error type.

Note that, technically speaking, all backends assume the module is valid. And this validity includes having a binding decoration on all resource globals. And now this code makes MSL backend also work correctly for a subset of modules that are valid except for the bindings part. So it's a tricky case, and it's not directly hit by WebGPU paths.

[teoxoy]

I don't see how this change "allows the MSL backend also work correctly for a subset of modules that are valid except for the bindings part". To me it only looks like it introduces more granular errors.

[kvark]

@teoxoy let me try to explain differently.
Imagine your have a naga module. You are interested in a specific entry point, to convert it to MSL.
Do we care about @group and @binding decorations on the globals not used by the entry point? If we strictly follow WGSL spec, then the validator should not pass such module as a whole. It requires resource decorations to be placed, and to be non-conflicting. But the MSL backend is not a validator and shouldn't be. It assumes the module is validated. But if it's not using some of the globals, there is no strong reason for it to pay attention to what decorations those globals have.

Currently, it pays attention - it panics if there is a missing resource decoration on a global that it doesn't even use. This is accidental, not intentional. With this PR, it will no longer panic, but instead will happily produce the MSL code.

[teoxoy]

Currently, it pays attention - it panics if there is a missing resource decoration on a global that it doesn't even use.

Where does this happen in the code? Could you point me to it?

This unwrap

naga/src/back/msl/writer.rs

Line 3784 in 5a1f43d

let resolved = options.resolve_resource_binding(ep.stage, binding).unwrap();

is gated by fun_info[handle].is_empty()

naga/src/back/msl/writer.rs

Lines 3752 to 3756 in 5a1f43d

	for (handle, var) in module.global_variables.iter() {
	let usage = fun_info[handle];
	if usage.is_empty() {
	continue;
	}

and the body of this loop is also gated by fun_info[var_handle].is_empty()

naga/src/back/msl/writer.rs

Lines 3397 to 3401 in 5a1f43d

	if !options.fake_missing_bindings {
	for (var_handle, var) in module.global_variables.iter() {
	if fun_info[var_handle].is_empty() {
	continue;
	}

Besides the more granular error, behavior wise, this PR seems to only ignore globals in Function (this should be unreachable), Private and WorkGroup address spaces if !options.fake_missing_bindings.

Am I missing something?

[kvark]

@teoxoy thank you for looking deeply into this!
The annoying panic is here -

naga/src/back/msl/writer.rs

Line 3687 in 8e1b052

.resolve_resource_binding(ep.stage, var.binding.as_ref().unwrap())

Note the comment above:

// the resolves have already been checked for !fake_missing_bindings case

You are correct that there is a check in the loop before this line:

                let usage = fun_info[handle];
                if usage.is_empty() || var.space == crate::AddressSpace::Private {
                    continue;
                }

However, the problem is - it panics while trying to output another entry point, not the one we are interested in.
Generally speaking, the logic of the MSL backend goes as:

• export all the functions that use available globals. Other functions are skipped, see comment in

  naga/src/back/msl/writer.rs

  Line 3395 in 8e1b052

  // skip this entry point if any global bindings are missing,

• where the "availability" is determined by the combination of types and resource overrides (in msl::Options).

This PR extends this definition by also considering a global variable to not be "available" if it's missing resource binding decorations. That's something the old code path didn't expect, but it's a useful thing to consider for my case.

[teoxoy]

Ah, I see how this is supposed to work now!
Thanks for bearing with me 😅

Before this PR, we were ignoring a var.binding being None in the if !options.fake_missing_bindings { block
causing a panic later when we do var.binding.as_ref().unwrap()
and we now error instead with MissingBinding.