Bug 100303 - Adding a single, meaningless if-else to a shader source leads to different image
Summary: Adding a single, meaningless if-else to a shader source leads to different image
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-21 15:07 UTC by Hugues Evrard
Modified: 2017-04-08 01:25 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Zip archive with files to reproduce (69.51 KB, application/zip)
2017-03-21 15:07 UTC, Hugues Evrard
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hugues Evrard 2017-03-21 15:07:49 UTC
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
Comment 1 Samuel Pitoiset 2017-03-22 01:04:30 UTC
Hi Hugues, thanks for reporting this.

Reassigned to glsl-compiler because this can be reproduced with Nouveau and llvmpipe.
Comment 2 Roland Scheidegger 2017-03-26 00:06:12 UTC
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...
Comment 3 Roland Scheidegger 2017-03-26 01:04:10 UTC
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)) )
    ))

)
Comment 4 Timothy Arceri 2017-03-26 03:11:48 UTC
(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
Comment 5 Roland Scheidegger 2017-03-26 03:51:19 UTC
(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.
Comment 6 Timothy Arceri 2017-03-26 12:36:02 UTC
This fixes the piglit tests: https://patchwork.freedesktop.org/patch/146459/

However we need a more complex fix in order to handle return values.
Comment 7 Timothy Arceri 2017-04-07 01:29:39 UTC
Fix sent to the list:

https://patchwork.freedesktop.org/patch/148970/
Comment 8 Timothy Arceri 2017-04-08 01:25:17 UTC
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.