Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLSL back end may duplicate unpack2x16float arguments #2123

Closed
jimblandy opened this issue Nov 8, 2022 · 3 comments
Closed

HLSL back end may duplicate unpack2x16float arguments #2123

jimblandy opened this issue Nov 8, 2022 · 3 comments
Labels
area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language

Comments

@jimblandy
Copy link
Member

jimblandy commented Nov 8, 2022

#2002 added support for WGSL's unpack2x16float function to the HLSL back end, but the code there may result in code for the argument expression being duplicated. The HLSL back end code reads:

  Function::Unpack2x16float => {
      write!(self.out, "float2(f16tof32(")?;
      self.write_expr(module, arg, func_ctx)?;
      write!(self.out, "), f16tof32((")?;
      self.write_expr(module, arg, func_ctx)?;
      write!(self.out, ") >> 16))")?;
  }

If arg is a baked expression, then this will be fine, but there's no way for the analyzer to know that the HLSL back end intends to use this expression twice, so its reference count could be one. The result would be that a call like:

unpack2x16float(some expensive expr)

would generate HLSL like:

float2(f16tof32(some expensive expr), f16to32((some expensive expr) >> 16))

The impact of this is limited, though: Naga IR ensures that the duplicated expression will never have function calls or side effects. And the result may be cleaned up by the HLSL compiler anyway. But it seemed unintentional.

The fix would probably be a pre-pass over the expressions in the HLSL back end to bump the reference count of expression that appear as the operand of unpack2x64float, or otherwise indicate that they need to be baked.

cc @expenses

@jimblandy jimblandy added area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language labels Nov 8, 2022
@teoxoy
Copy link
Member

teoxoy commented Nov 9, 2022

I'm thinking injecting a function to avoid this issue would be nicer. Thoughts?

@teoxoy
Copy link
Member

teoxoy commented Nov 10, 2022

We seem to be already baking expressions for dot in the msl and glsl backends.

But I still feel like polyfilling those via functions in the output would be nicer. Have we considered this previously?

@teoxoy
Copy link
Member

teoxoy commented Jan 31, 2023

Fixed by #2226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language
Projects
None yet
Development

No branches or pull requests

2 participants