[spv-in] Properly parse empty names and set entry point function name #789
Conversation
src/front/spv/mod.rs
Outdated
| @@ -2371,7 +2371,8 @@ impl<I: Iterator<Item = u32>> Parser<I> { | |||
| if left != 0 { | |||
| return Err(Error::InvalidOperand); | |||
| } | |||
| self.future_decor.entry(id).or_default().name = Some(name); | |||
| self.future_decor.entry(id).or_default().name = | |||
| if name.is_empty() { None } else { Some(name) }; | |||
There was a problem hiding this comment.
I don't think this is needed. We are parsing OpName here. If the incoming SPIRV has an empty name, so be it.
There was a problem hiding this comment.
It's better to fix this issue in backend side like we discuss in matrix? Otherwise we have variables with name _. quad-vert.msl in diff for example.
There was a problem hiding this comment.
I think it better to fix in Namer, not in spv frontend. Will do in next PR.
There was a problem hiding this comment.
Does this actually need to be solved? If the backend ends up generating the name "_", so be it, given that the input annotated something with an empty string.
There was a problem hiding this comment.
Does this actually need to be solved?
It's only about readability.
gl_PerVertex _ = {};
type10 {v_uv, _.gl_Position, _.gl_PointSize, {_.gl_ClipDistance[0]}};gl_PerVertex global = {};
type10 {v_uv, global.gl_Position, global.gl_PointSize, {global.gl_ClipDistance[0]}};There was a problem hiding this comment.
Ok. I think we can assume that an empty name (that we want to handle) is only coming from SPIR-V, and only from the gl_PerVertex struct. Could we then have the code in place in spv-in specifically that is as narrow (for this use case) as possible? I don't think we want any sort of generic mechanism to eliminate empty names, that doesn't make sense. So a very specialized piece in the spv-in for this gl_PerVertex struct should be most appropriate.
There was a problem hiding this comment.
Ok, I decided to fix it on namer side. If it's ok, we can merge PR, or we or can leave it as is and close PR. It's not such an important issue.
src/front/spv/mod.rs
Outdated
| @@ -2371,7 +2371,8 @@ impl<I: Iterator<Item = u32>> Parser<I> { | |||
| if left != 0 { | |||
| return Err(Error::InvalidOperand); | |||
| } | |||
| self.future_decor.entry(id).or_default().name = Some(name); | |||
| self.future_decor.entry(id).or_default().name = | |||
| if name.is_empty() { None } else { Some(name) }; | |||
There was a problem hiding this comment.
It's better to fix this issue in backend side like we discuss in matrix? Otherwise we have variables with name _. quad-vert.msl in diff for example.
src/proc/namer.rs
Outdated
| @@ -57,7 +57,14 @@ impl Namer { | |||
|
|
|||
| fn call_or(&mut self, label: &Option<String>, fallback: &str) -> String { | |||
| self.call(match *label { | |||
| Some(ref name) => name, | |||
| Some(ref name) => { | |||
| // Name can be empty for some cases in SPIR-V | |||
There was a problem hiding this comment.
there is only one case where this happens in practice, and spv frontend should be handling it, not Namer
There was a problem hiding this comment.
One more try. Thank you for your patience 😄
I'm not sure about these changes at all. Found these issues when developing
wgsl-out.