Bug 62357 - llvmpipe: Fragment Shader with "return" in main causes back output
llvmpipe: Fragment Shader with "return" in main causes back output
Status: RESOLVED FIXED
Product: Mesa
Classification: Unclassified
Component: Other
unspecified
Other All
: medium normal
Assigned To: mesa-dev
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-14 21:38 UTC by drago01
Modified: 2013-03-22 21:07 UTC (History)
1 user (show)

See Also:


Attachments
piglit shader test to exercise the bug (336 bytes, text/plain)
2013-03-14 22:18 UTC, Brian Paul
Details
another testcase (480 bytes, text/plain)
2013-03-16 00:43 UTC, Roland Scheidegger
Details

Note You need to log in before you can comment on or make changes to this bug.
Description drago01 2013-03-14 21:38:04 UTC
We used a fragment shader in gnome-shell where we bail out early using
 if (...)
   return;
inside main() ... the resulting color ended up being black even though the color has been set to a value before the if statement.

See https://bugzilla.gnome.org/show_bug.cgi?id=695607 for details.
Comment 1 Brian Paul 2013-03-14 22:18:55 UTC
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
Comment 2 Brian Paul 2013-03-14 22:20:49 UTC
(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.
Comment 3 drago01 2013-03-14 22:59:28 UTC
(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.)
Comment 4 drago01 2013-03-14 23:06:57 UTC
FWIW this was using LLVM 3.2 as well.
Comment 5 José Fonseca 2013-03-15 11:50:22 UTC
(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?
Comment 6 Brian Paul 2013-03-15 12:44:08 UTC
My piglit test was with LLVM 3.2.
Comment 7 Roland Scheidegger 2013-03-15 13:43:15 UTC
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.
Comment 8 Roland Scheidegger 2013-03-15 18:17:35 UTC
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).
Comment 9 Brian Paul 2013-03-15 18:57:43 UTC
Nice find, Roland.  Do you want fix up my test case and put it into piglit?
Comment 10 Roland Scheidegger 2013-03-15 19:35:13 UTC
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...
Comment 11 Roland Scheidegger 2013-03-16 00:43:33 UTC
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...).
Comment 12 Roland Scheidegger 2013-03-22 19:41:56 UTC
This is fixed with 5af7b45986d1b56c568ebe9c3a40d48853e2e9ff.
Not sure if the testcase would be that useful as a piglit test.
Comment 13 José Fonseca 2013-03-22 20:11:59 UTC
(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?
Comment 14 Roland Scheidegger 2013-03-22 20:49:19 UTC
(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.
Comment 15 José Fonseca 2013-03-22 21:07:33 UTC
(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.