Bug 100303

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