Skip to content

Commit

Permalink
Do not add bindings LiteralToUniformReductionOpportunity
Browse files Browse the repository at this point in the history
The LiteralToUniformReductionOpportunity adds new uniforms to a shader
job.  It should not provide these uniforms with bindings -- it is up
to the reducer to add bindings if they are deemed necessary, right
before emitting the shader as a text file.

This change fixes that issue.  However, it turns out that the reducer
was not adding bindings in the case that no uniforms were present in
the first place -- it was treating a uniform-free shader as a
non-Vulkan shader.  As a temporary workaround, a uniform-free shader
is now treated by the reducer as a Vulkan shader so that if uniforms
are then created, they get bindings when written out as text files.

Related issue to fix this more elegantly in the long-run: #1046.
  • Loading branch information
afd committed Jul 30, 2020
1 parent ef5ef2f commit a5e8a3d
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,12 @@ public String doReduction(
int stepLimit) throws IOException {

// This is used for Vulkan compatibility.
final boolean requiresUniformBindings = initialState.hasUniformBindings();
// TODO(https://github.com/google/graphicsfuzz/issues/1046): The check for zero uniforms is a
// workaround for the fact that we don't have a way to infer what the right thing to do is
// when there are no uniforms. As per the issue, we should really have a --vulkan option that
// instructs the reducer as to whether we want Vulkan-style uniform blocks.
final boolean requiresUniformBindings =
initialState.getPipelineInfo().getNumUniforms() == 0 || initialState.hasUniformBindings();
final Optional<String> pushConstant = initialState.getPushConstant();
if (initialState.hasUniformBindings()) {
// We eliminate uniform bindings while applying reduction steps, and re-introduce them
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ void applyReductionImpl() {
if (!shaderJob.getPipelineInfo().hasUniform(arrayName)) {
shaderJob.getPipelineInfo().addUniform(arrayName, basicType,
Optional.of(0), new ArrayList<>());
shaderJob.getPipelineInfo().addUniformBinding(arrayName, false,
shaderJob.getPipelineInfo().getUnusedBindingNumber());
}

final int index;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,7 @@ public void testReplaceUInts() throws Exception {
* opportunity at first. Then checks that the one shader had the array uniform added,
* and the other shader stays the same. After that, another uniform is manually added to
* the pipeline info and the rest of the opportunities are applied. The test succeeds if
* the bindings have not changed, the amount of opportunies is correct and literals in the
* shaders are replaced as expected.
* the number of opportunities is correct and literals in the shaders are replaced as expected.
*/
@Test
public void testManuallyAddedUniform() throws Exception {
Expand Down Expand Up @@ -396,13 +395,8 @@ public void testManuallyAddedUniform() throws Exception {
CompareAsts.assertEqualAsts(fragmentShaderNotReplaced, shaderJob.getFragmentShader().get());
assertEquals(1, pipelineInfo.getNumUniforms());

final int bindingNumber = shaderJob.getPipelineInfo().getBinding("_GLF_uniform_int_values");

final int nextUnusedBindingNumber = shaderJob.getPipelineInfo().getUnusedBindingNumber();
shaderJob.getPipelineInfo().addUniform("TEST", BasicType.INT,
Optional.of(0), new ArrayList<>());
shaderJob.getPipelineInfo().addUniformBinding("TEST", false,
nextUnusedBindingNumber);

ops.forEach(AbstractReductionOpportunity::applyReduction);
assertEquals("There should be three opportunities", 3, ops.size());
Expand All @@ -411,10 +405,6 @@ public void testManuallyAddedUniform() throws Exception {
CompareAsts.assertEqualAsts(vertexShaderReplaced2, shaderJob.getVertexShader().get());
CompareAsts.assertEqualAsts(fragmentShaderReplaced, shaderJob.getFragmentShader().get());

// check that the binding numbers remain the same.
assertEquals(bindingNumber, shaderJob.getPipelineInfo().getBinding("_GLF_uniform_int_values"));
assertEquals(nextUnusedBindingNumber, shaderJob.getPipelineInfo().getBinding("TEST"));

// Check that the uniforms won't be recursively replaced in the next pass.
final List<LiteralToUniformReductionOpportunity> ops2 =
LiteralToUniformReductionOpportunities
Expand Down

0 comments on commit a5e8a3d

Please sign in to comment.