From a5e8a3db0a3d752b33ca2df8bf8435faf66605cd Mon Sep 17 00:00:00 2001 From: Alastair Donaldson Date: Fri, 31 Jul 2020 00:59:57 +0100 Subject: [PATCH] Do not add bindings LiteralToUniformReductionOpportunity 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. --- .../com/graphicsfuzz/reducer/ReductionDriver.java | 7 ++++++- .../LiteralToUniformReductionOpportunity.java | 2 -- .../LiteralToUniformReductionOpportunitiesTest.java | 12 +----------- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/reducer/src/main/java/com/graphicsfuzz/reducer/ReductionDriver.java b/reducer/src/main/java/com/graphicsfuzz/reducer/ReductionDriver.java index e6ac82b0c..8579c1c94 100755 --- a/reducer/src/main/java/com/graphicsfuzz/reducer/ReductionDriver.java +++ b/reducer/src/main/java/com/graphicsfuzz/reducer/ReductionDriver.java @@ -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 pushConstant = initialState.getPushConstant(); if (initialState.hasUniformBindings()) { // We eliminate uniform bindings while applying reduction steps, and re-introduce them diff --git a/reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunity.java b/reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunity.java index 4c94c6c1e..0872a687a 100644 --- a/reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunity.java +++ b/reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunity.java @@ -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; diff --git a/reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunitiesTest.java b/reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunitiesTest.java index 5881993d2..1d059e91b 100644 --- a/reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunitiesTest.java +++ b/reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/LiteralToUniformReductionOpportunitiesTest.java @@ -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 { @@ -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()); @@ -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 ops2 = LiteralToUniformReductionOpportunities