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

Switch nodes no longer cull unused branches in shader generation #1526

Closed
kwokcb opened this issue Sep 9, 2023 · 5 comments
Closed

Switch nodes no longer cull unused branches in shader generation #1526

kwokcb opened this issue Sep 9, 2023 · 5 comments

Comments

@kwokcb
Copy link
Contributor

kwokcb commented Sep 9, 2023

With this PR, switch nodes no longer optimize to use only 1 branch.

The previous logic checks for an unconnected constant value which is used to perform the optimization but was removed.
The resulting generated code is now larger since this was removed.

@kwokcb kwokcb changed the title CodeGen Regression: switch nodes no log optimize to single branches in codegen CodeGen: Regression -- switch nodes no log optimize to single branches in codegen Sep 9, 2023
@jstone-lucasfilm
Copy link
Member

This is a good area for discussion, though we may want to consider this category of optimization as a future part of our "interface reduction" rules for shader generation, rather than making a special case for the switch node.

Culling generated code based on a constant input isn’t always what the user will want, as this requires shader recompilation when inputs are modified in an editor, and it seems worthwhile to make this behavior controllable by the client application.

@jstone-lucasfilm jstone-lucasfilm changed the title CodeGen: Regression -- switch nodes no log optimize to single branches in codegen Switch nodes no longer cull unused branches in shader generation Sep 9, 2023
@JGamache-autodesk
Copy link
Contributor

The culling behavior of the switch node was a bug introduced way back in 1.38.0 that was fixed in 1.38.8.

If you look at the node definition of the switch node:

  <nodedef name="ND_switch_float" node="switch" nodegroup="conditional">
    <input name="in1" type="float" value="0.0" />
    <input name="in2" type="float" value="0.0" />
    <input name="in3" type="float" value="0.0" />
    <input name="in4" type="float" value="0.0" />
    <input name="in5" type="float" value="0.0" />
    <input name="which" type="float" value="0.0" />
    <output name="out" type="float" defaultinput="in1" />
  </nodedef>

You will see no hint that leaving the which input unconnected will result in branch pruning.

I expect, given this node definition, that my shader will have a shader uniform parameter for the which input that can be driven programmatically, allowing the same shader code to be re-used between shader instance differing only on the value of that parameter.

This would make it consistent with the behavior of 99% of the nodes. The constant node being the one and only exception.

Now, if I were to connect a constant node to the which input, I could expect that a good constant propagation pass on the graph would end up pruning the switch node, but in this case, the user explicitly requested this behavior by connecting a constant node, it was not explicitly done without the user's consent.

My vote is to close this issue as "by design" and move on to future optimization code in the shadergen to properly handle constant propagation.

@jstone-lucasfilm
Copy link
Member

@JGamache-autodesk This sounds like a fair take to me, and I'd be interested in thoughts from @kwokcb.

@kwokcb
Copy link
Contributor Author

kwokcb commented Sep 15, 2023

Sorry, if this is long winded...

Some additional context to consider:

  1. Is the input "exposed" to the user to modify. If not then, we should consider this to be something that can be optimized out. For example if it's inside a nodegraph for a definition.
  2. Code generation allows for exposure for node inputs internal to a graph or not. If not exposed then this should be an optimization consideration.
  3. Performance should be considered for always having if or switch code being emitted. For example do we know that this works well for ESSL. So the optimization probably should allow for generator specific optimizations.
  4. I'm not sure what this means for languages like MDL and OSL. Both are not directly executable so need to be parsed. In that context how much reuse is there as these are not strictly "shader code". e.g. See Ashwin's "uniform" poking function which has no OSL equivalent currently (Add method to update uniforms in ShaderRenderer #1510). It would be good to see some context added here.

I am not so sure about "guessing" what the user wants as there are different scenarios for code generation and not just reuse for an edit workflow. I'm not sure you want to force a constant to be added by the user. This forces artists to consider shader optimization which could be the case but depends on how technical they are -- also as noted this may not apply to all backend generators.

I agree that performing branch pruning "under the covers" is also not a good solution but I was mostly worried that behaviour has changed. It was always intentional to prune (even before 1.38) as optimization was the only factor considered. Reversing this without an new option I don't see as any better, so agree that explicit optimization options should be added.

BTW, in addition to optimization for reuse I think that the optimization option used to produce a shader should be recorded on the Shader itself as the optimization option can be change dynamically.

@jstone-lucasfilm
Copy link
Member

These are good thoughts for the future, @kwokcb, and I'd recommend that we focus on improvements to the client-side controls for generated shader interfaces in MaterialX, removing the need for heuristics that apply a single set of rules to all users. I'll close out this issue for now, and am looking forward to future discussions in this space.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants