Created attachment 130352 [details] Zip archive with files to reproduce See: https://github.com/mc-imperial/shader-compiler-bugs/issues/62 Steps to Reproduce: ------------------------------- 0. This bug seems to affect AMD GPU (tested on Tonga, see below for more config details) 1. Obtain and build the latest release of get-image, a simple tool that creates a .png image from a fragment shader https://github.com/mc-imperial/get-image 2. From a terminal, execute: /path/to/get_image/linux_out/install/bin/get_image original.frag This will create output.png, which should look like original.png 3. From a terminal, execute: /path/to/get_image/linux_out/install/bin/get_image variant.frag This will create output.png, which should look like variant.png Expected Results: ------------------------------- Both shaders should render an image that looks like original.png. This is because the only difference between the shader source files is the addition of an if-else whose if block is empty and else block contains the original code. We control the condition value to make sure it is always false at runtime. Diff: ``` 27a28,29 > uniform vec2 injectionSwitch; > 117a120,124 > if(injectionSwitch.x > injectionSwitch.y) > { > } > else > { 129a137 > } ``` The injectionSwitch uniform is always set to { 0.0, 1.0 }. The else block contains the original code, untouched. Actual Results: ------------------------------- The shaders render different images, which they should not: the shadow in the lower part of the ball disappears in the variant. Additional Information: ------------------------------- Some info on the system where the bug was found: - CPU: AMD A10-7850K Radeon R7, 12 Compute Cores 4C+8G - GPU: Advanced Micro Devices, Inc. [AMD/ATI] Tonga PRO [Radeon R9 285/380] - Mesa: 17.1.0-devel (git-08df015) - OS: Ubuntu 16.04, Linux 4.4.0-67-generic
Hi Hugues, thanks for reporting this. Reassigned to glsl-compiler because this can be reproduced with Nouveau and llvmpipe.
This is the output of glsl_compiler --dump_lir --version=100 original.frag for the getHardShadow function: ( function getHardShadow (signature float (parameters (declare (in ) vec3 ro) (declare (in ) vec3 rd) ) ( (declare (temporary ) float return_value) (declare (temporary ) bool return_flag) (assign (x) (var_ref return_flag) (constant bool (0)) ) (declare () int i) (declare () float depth) (assign (x) (var_ref depth) (constant float (0.010000)) ) (assign (x) (var_ref i) (constant int (0)) ) (loop ( (if (expression bool >= (var_ref i) (constant int (30)) ) ( break ) ()) (declare (temporary ) float sceneDist_retval) (call sceneDist (var_ref sceneDist_retval) ((expression vec3 + (var_ref ro) (expression vec3 * (var_ref rd) (var_ref depth) ) ) )) (if (expression bool < (var_ref sceneDist_retval) (constant float (0.010000)) ) ( (assign (x) (var_ref return_value) (var_ref shadowIntensity) ) (assign (x) (var_ref return_flag) (constant bool (1)) ) break ) ()) (assign (x) (var_ref depth) (expression float + (var_ref depth) (var_ref sceneDist_retval) ) ) (assign (x) (var_ref i) (expression int + (var_ref i) (constant int (1)) ) ) )) (if (expression bool ! (var_ref return_flag) ) ( (assign (x) (var_ref return_value) (constant float (1.000000)) ) (assign (x) (var_ref return_flag) (constant bool (1)) ) ) ()) (return (var_ref return_value) ) )) And for variant.frag: ( function getHardShadow (signature float (parameters (declare (in ) vec3 ro) (declare (in ) vec3 rd) ) ( (declare () float depth) (assign (x) (var_ref depth) (constant float (0.010000)) ) (if (expression bool <= (swiz x (var_ref injectionSwitch) )(swiz y (var_ref injectionSwitch) )) ( (declare () int i) (assign (x) (var_ref i) (constant int (0)) ) (loop ( (if (expression bool >= (var_ref i) (constant int (30)) ) ( break ) ()) (declare (temporary ) float sceneDist_retval) (call sceneDist (var_ref sceneDist_retval) ((expression vec3 + (var_ref ro) (expression vec3 * (var_ref rd) (var_ref depth) ) ) )) (if (expression bool < (var_ref sceneDist_retval) (constant float (0.010000)) ) ( break ) ()) (assign (x) (var_ref depth) (expression float + (var_ref depth) (var_ref sceneDist_retval) ) ) (assign (x) (var_ref i) (expression int + (var_ref i) (constant int (1)) ) ) )) ) ()) (return (constant float (1.000000)) ) )) ) So, for some reason the compiler completely eliminated the conditional return in the loop inside the branch and figured it will just unconditionally return the value corresponding to not meeting that condition in the loop in the original code. (In fact all code in this function with the exception of the return statement is now effectively dead.) Not sure though which lowering stage did this damage...
Here's a simplified example, not even the empty branch is necessary, it is sufficient if the return is inside a loop which is inside a conditional (I'm actually wondering why this isn't hit in ordinary shaders...): #version 130 uniform vec2 val_cond; float func1() { if(val_cond.x > val_cond.y) { for(int i = 0; i < 1; i++) { return 2.0; } } return 1.0; } void main(void) { gl_FragColor = vec4(func1()); } The --dump-lir output for func1 is just: ( function func1 (signature float (parameters ) ( (return (constant float (1.000000)) ) )) )
(In reply to Roland Scheidegger from comment #2) > > So, for some reason the compiler completely eliminated the conditional > return in the loop inside the branch and figured it will just > unconditionally return the value corresponding to not meeting that condition > in the loop in the original code. (In fact all code in this function with > the exception of the return statement is now effectively dead.) > Not sure though which lowering stage did this damage... I think there is a bug in do_lower_jumps(). I noticed a similar bug when working on loop unrolling for NIR. I wrote the following tests that pass on NIR unrolling/return lower but fail in GLSL IR. piglit/tests/spec/glsl-1.10/execution/vs-nested-return-sibling-loop.shader_tes piglit/tests/spec/glsl-1.10/execution/vs-nested-return-sibling-loop2.shader_test
(In reply to Timothy Arceri from comment #4) > (In reply to Roland Scheidegger from comment #2) > > > > So, for some reason the compiler completely eliminated the conditional > > return in the loop inside the branch and figured it will just > > unconditionally return the value corresponding to not meeting that condition > > in the loop in the original code. (In fact all code in this function with > > the exception of the return statement is now effectively dead.) > > Not sure though which lowering stage did this damage... > > I think there is a bug in do_lower_jumps(). I noticed a similar bug when > working on loop unrolling for NIR. I wrote the following tests that pass on > NIR unrolling/return lower but fail in GLSL IR. > > piglit/tests/spec/glsl-1.10/execution/vs-nested-return-sibling-loop. > shader_tes > piglit/tests/spec/glsl-1.10/execution/vs-nested-return-sibling-loop2. > shader_test Yes, that looks similar. Thease also have a return in a loop inside a conditional. Albeit the test case here even works without the sibling loop.
This fixes the piglit tests: https://patchwork.freedesktop.org/patch/146459/ However we need a more complex fix in order to handle return values.
Fix sent to the list: https://patchwork.freedesktop.org/patch/148970/
Fixed by: commit bfabef0e7104dc4043a74ef44e71ecc7636cfe12 Author: Timothy Arceri <tarceri@itsqueeze.com> Date: Fri Apr 7 11:24:37 2017 +1000 glsl: fix lower jumps for nested non-void returns Fixes the case were a loop contains a return and the loop is nested inside an if. Reviewed-by: Roland Scheidegger <sroland@vmware.com> https://bugs.freedesktop.org/show_bug.cgi?id=100303
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.