Bug 62357

Created attachment 76543 [details]
piglit shader test to exercise the bug

Do you happen to know which version of Mesa this is happening with?  I can't tell from the gnome bug.

I created a piglit shader test (attached) to exercise this and it works fine with Mesa 9.0 and later (at least).

Run with:
bin/shader_runner tests/spec/glsl-1.10/execution/fs-return-in-main.shader_test -auto

(In reply to comment #1)

> Run with:
> bin/shader_runner
> tests/spec/glsl-1.10/execution/fs-return-in-main.shader_test -auto

Err, just:
bin/shader_runner fs-return-in-main.shader_test -auto

If this turns out to be useful, I'll commit it to piglit at the above location.

(In reply to comment #1)
> Created attachment 76543 [details]
> piglit shader test to exercise the bug
> 
> Do you happen to know which version of Mesa this is happening with?  I can't
> tell from the gnome bug.

mesa-libGL-9.1-0.4.fc19.x86_64

> I created a piglit shader test (attached) to exercise this and it works fine
> with Mesa 9.0 and later (at least).
> 
> Run with:
> bin/shader_runner
> tests/spec/glsl-1.10/execution/fs-return-in-main.shader_test -auto

Can't do that right now (no package for fedora and no time to build piglit from source right now, can try to test this weekend.)

FWIW this was using LLVM 3.2 as well.

(In reply to comment #4)
> FWIW this was using LLVM 3.2 as well.

It could be a LLVM version specific issue.  I haven't done much testing with LLVM 3.2 (still using 3.1).  Could you re-try with LLVM 3.1?

My piglit test was with LLVM 3.2.

After a quick look at the generated IR, it indeed seems broken.
However, it is broken the opposite way, that is the early exit path is ok but if the path isn't taken it will pick zero as the output color.
So if you swap the test like so:
[test]
uniform vec4 v 1 0 1 0

Then it fails.

tgsi looks like this:
  0: SLT TEMP[0].x, CONST[0].xxxx, IMM[0].xxxx
  1: F2I TEMP[0].x, -TEMP[0]
  2: SLT TEMP[1].x, IMM[0].yyyy, CONST[0].yyyy
  3: F2I TEMP[1].x, -TEMP[1]
  4: OR TEMP[1].x, TEMP[0].xxxx, TEMP[1].xxxx
  5: SLT TEMP[2].x, CONST[0].zzzz, IMM[0].xxxx
  6: F2I TEMP[2].x, -TEMP[2]
  7: OR TEMP[1].x, TEMP[1].xxxx, TEMP[2].xxxx
  8: IF TEMP[1].xxxx :0
  9:   MOV_SAT OUT[0], CONST[0]
 10:   RET
 11: ENDIF
 12: ADD TEMP[0], IMM[0].xxxx, -CONST[0]
 13: MOV_SAT OUT[0], TEMP[0]
 14: END

And the IR part like this:
  %53 = fcmp une <8 x float> %52, zeroinitializer
  %54 = sext <8 x i1> %53 to <8 x i32>
// %54 is the result of the if condition of line 8
...
  %70 = call <8 x float> @llvm.x86.avx.max.ps.256(<8 x float> %57, <8 x float> zeroinitializer)
  %71 = call <8 x float> @llvm.x86.avx.min.ps.256(<8 x float> %70, <8 x float> <float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 1.000000e+00, float 1.000000e+00>)
// %71 contains mov_sat of line 9
  %72 = bitcast <8 x i32> %54 to <8 x float>
// %72 is still condition of line 8 as float
  %73 = call <8 x float> @llvm.x86.avx.blendv.ps.256(<8 x float> zeroinitializer, <8 x float> %71, <8 x float> %72)
// %73 contains the output going to color pack - you can see that if the condition (%72) wasn't true, it simply selects zero.

Looks like we unconditionally return when we see a return not in a function.
Some trivial attempt at fixing that seems to work but I'm not quite sure if it's right or if we need to do more.

diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
index 0dc26b5..e56bb62 100644
--- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
+++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
@@ -348,7 +348,8 @@ static void lp_exec_mask_ret(struct lp_exec_mask *mask, int *pc)
    LLVMBuilderRef builder = mask->bld->gallivm->builder;
    LLVMValueRef exec_mask;
 
-   if (mask->call_stack_size == 0) {
+   if (mask->call_stack_size == 0 &&
+       mask->cond_stack_size == 0) {
       /* returning from main() */
       *pc = -1;
       return;

I guess might also need to check loop_stack_size too (could have a loop which never executes).

Nice find, Roland.  Do you want fix up my test case and put it into piglit?

Hmm I just noticed this doesn't actually work. Instead of not executing the code after the conditional, the result will now always be as if the condition passed, so the original testcase now fails (and the IR clearly indicates llvm dropped all the code for the if condition (that is the comparison instructions are still there but that's just because the results of that are stored back to the register file).
I can't quite see though why if this doesn't work when happening in main, how could it possibly work in a subroutine. I think there's something wrong with handling condition masks around function calls/rets. Can't quite see yet how this is all supposed to work together...

Created attachment 76596 [details]
another testcase

Here's another test case. With this one you can actually see if both the early exit and the code after the if are executed properly (as it outputs green on odd pixels and pink on even ones).
(Requires glsl 1.30 though now.)
Succeeds with softpipe.
With unpatched llvmpipe (that one definitely can't work), it fails on the pink pixels (as it never executes the code after the ret, hence it outputs green/black pattern).
With the patch, it will fail on the green pixels (outputs a solid pink - this is less obvious why...).

This is fixed with 5af7b45986d1b56c568ebe9c3a40d48853e2e9ff.
Not sure if the testcase would be that useful as a piglit test.

(In reply to comment #12)
> This is fixed with 5af7b45986d1b56c568ebe9c3a40d48853e2e9ff.
> Not sure if the testcase would be that useful as a piglit test.

Why not?

(In reply to comment #13)
> (In reply to comment #12)
> > This is fixed with 5af7b45986d1b56c568ebe9c3a40d48853e2e9ff.
> > Not sure if the testcase would be that useful as a piglit test.
> 
> Why not?

Yeah hmm maybe it would be useful.
Any ideas where it should be added?
It's a crappy test though, Brian's version didn't test both the early exit taken or not path, whereas my version does but requires glsl 1.30 even though the bug itself has nothing to do with glsl 1.30.

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > This is fixed with 5af7b45986d1b56c568ebe9c3a40d48853e2e9ff.
> > > Not sure if the testcase would be that useful as a piglit test.
> > 
> > Why not?
> 
> Yeah hmm maybe it would be useful.
> Any ideas where it should be added?

No. Choose somwhere you think is good. If people feel strongly about I'm sure they'll voice their opinion on the review request.

> It's a crappy test though, Brian's version didn't test both the early exit
> taken or not path, whereas my version does but requires glsl 1.30 even
> though the bug itself has nothing to do with glsl 1.30.

If it tests a bug that was not shown at till now then it is a superb test. Everything else is secondary.