Bug 34201 - [GLSL] fail to use >16 varying floats
Summary: [GLSL] fail to use >16 varying floats
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: All Linux (All)
: medium major
Assignee: Ian Romanick
QA Contact: fangxun
URL:
Whiteboard:
Keywords:
: 43121 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-02-12 00:11 UTC by Gordon Jin
Modified: 2014-07-18 02:56 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
new piglit test case (780 bytes, text/plain)
2011-02-12 00:15 UTC, Gordon Jin
Details
Failing shader from civ4 (16.52 KB, text/plain)
2011-11-27 16:31 UTC, Tom Stellard
Details

Description Gordon Jin 2011-02-12 00:11:54 UTC
The attached case uses 32 varying floats (actually only 17 varying floats could trigger this). It gets assertion with i965 driver and fails with swrast.

piglit/bin/shader_runner -auto glsl-varying-32.shader_test
libGL: OpenDriver: trying /opt/X11R7/lib/dri/i965_dri.so
shader_runner: brw_vs_emit.c:1457: get_dst: Assertion `c->regs[dst.File][dst.Index].nr != 0' failed.
Aborted (core dumped)

(gdb) bt
#0  0xb7fff424 in __kernel_vsyscall ()
#1  0x42c0fd71 in raise () from /lib/libc.so.6
#2  0x42c1164a in abort () from /lib/libc.so.6
#3  0x42c08de8 in __assert_fail () from /lib/libc.so.6
#4  0xb7a6c715 in get_dst (c=<value optimized out>, dst=...)
    at brw_vs_emit.c:1457
#5  0xb7a7160b in brw_vs_emit (c=0xbffd41dc) at brw_vs_emit.c:1936
#6  0xb7a6adeb in do_vs_prog (brw=0x806a900, vp=<value optimized out>,
    key=0xbfffee68) at brw_vs.c:85
#7  0xb7a6b02d in brw_upload_vs_prog (brw=0x806a900) at brw_vs.c:151
#8  0xb7a6a0c7 in brw_validate_state (brw=0x806a900) at brw_state_upload.c:400
#9  0xb7a59bb8 in brw_try_draw_prims (ctx=0x806a900, arrays=0x80ad518,
    prim=0xbfffefb8, nr_prims=1, ib=0x0, index_bounds_valid=1 '\001',
    min_index=0, max_index=3) at brw_draw.c:362
#10 brw_draw_prims (ctx=0x806a900, arrays=0x80ad518, prim=0xbfffefb8,
    nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=0, max_index=3)
    at brw_draw.c:447
#11 0xb7b44354 in vbo_draw_arrays (ctx=0x806a900, mode=<value optimized out>,
    start=0, count=4, numInstances=1) at vbo/vbo_exec_array.c:588
#12 0x0804e988 in piglit_draw_rect (x=-1, y=-1, w=2, h=2)
    at /root/gordon/piglit/tests/util/piglit-util.c:748
#13 0x0804c910 in piglit_display ()
    at /root/gordon/piglit/tests/shaders/shader_runner.c:724


# LIBGL_ALWAYS_SOFTWARE=1 piglit/bin/shader_runner -auto glsl-varying-32.shader_test
libGL: OpenDriver: trying /opt/X11R7/lib/dri/swrast_dri.so
Probe at (0,0)
  Expected: 0.320000 0.000000 0.000000 1.000000
  Observed: 0.160784 0.000000 0.000000 1.000000
PIGLIT: {'result': 'fail' }


The GLSL 1.10 spec mentions:
const int gl_MaxVaryingFloats = 32; // ARB_vertex_shader

I don't have history data to show if it's regression.
Comment 1 Gordon Jin 2011-02-12 00:15:10 UTC
Created attachment 43282 [details]
new piglit test case
Comment 2 Ian Romanick 2011-02-15 15:39:11 UTC
Currently the compiler doesn't do any packing of varyings.  Any shader that uses more than 16 varyings will currently fail.  This is probably fixable in the linker, but it's not currently on my schedule to fix.  I expect that it will be non-trivial.
Comment 3 Ian Romanick 2011-06-29 10:45:41 UTC
The crash should be fixed by the commit below.  This commit has also already been cherry picked to the 7.10 branch (85b965b).  The test case will still fail because it won't link.  This behavior is correct.

commit de77324d8f14951e4dc17f570e49451a0cd33121
Author: Ian Romanick <ian.d.romanick@intel.com>
Date:   Thu Jun 9 13:31:32 2011 -0700

    linker: Reject shaders that use too many varyings
    
    Previously it was up to the driver or later code generator to reject
    these shaders.  It turns out that nobody did this.
    
    This will need changes to support geometry shaders.
    
    NOTE: This is a candidate for the stable branches.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=37743
    Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Comment 4 Gordon Jin 2011-08-25 18:42:26 UTC
(In reply to comment #3)
> The crash should be fixed by the commit below.  This commit has also already
> been cherry picked to the 7.10 branch (85b965b).  The test case will still fail
> because it won't link.  This behavior is correct.

Verified the crash gone and the test case fails now.

Can you explain why this fail behavior is correct? Don't you think this test case valid?
Comment 5 Gordon Jin 2011-10-24 20:01:54 UTC
Ian, can you explain why the fail is correct?
Comment 6 Ian Romanick 2011-10-27 10:13:05 UTC
I had misunderstood the spec.  I had initially thought that the same counting rules for vertex attributes applied for varyings.  That is, the implementation is not required to do any packing.  This is not the case, and we do need to fix this.
Comment 7 Ian Romanick 2011-11-21 17:27:58 UTC
*** Bug 43121 has been marked as a duplicate of this bug. ***
Comment 8 Tom Stellard 2011-11-27 16:31:06 UTC
Created attachment 53891 [details]
Failing shader from civ4

Here is the civ4 shader from bug 43121 that fails with too many varyings.  Is this shader legal according to the spec?
Comment 9 Ian Romanick 2011-11-28 15:38:55 UTC
(In reply to comment #8)
> Created attachment 53891 [details]
> Failing shader from civ4
> 
> Here is the civ4 shader from bug 43121 that fails with too many varyings.  Is
> this shader legal according to the spec?

No, but also yes.

The no part:

The driver says that it supports only 32 varying components, but that shader, without additional optimizations, uses 36.

The yes part:

I don't think r300 should say that it only supports 32 varying components.  I think it supports more than that.  The value 32 comes from 8 texture coordinates times 4 components.  However, it also has at least 2 color varying slots.  If the fragment shader doesn't use gl_Color (or gl_SecondaryColor) those should be used as general varyings.

I haven't verified this by looking at the r300 documentation, but Apple, who is even more anal about the spec than I am, has advertised 40 varying components on that hardware since forever.  See http://developer.apple.com/graphicsimaging/opengl/capabilities/GLInfo_1058.html.  The value 40 comes from 8 texture coordinates plus two colors times 4 components.

The i915 driver currently has the same problem.
Comment 10 Tom Stellard 2011-12-01 15:53:51 UTC
(In reply to comment #9)

> I don't think r300 should say that it only supports 32 varying components.  I
> think it supports more than that.  The value 32 comes from 8 texture
> coordinates times 4 components.  However, it also has at least 2 color varying
> slots.  If the fragment shader doesn't use gl_Color (or gl_SecondaryColor)
> those should be used as general varyings.
> 
> I haven't verified this by looking at the r300 documentation, but Apple, who is
> even more anal about the spec than I am, has advertised 40 varying components
> on that hardware since forever.  See
> http://developer.apple.com/graphicsimaging/opengl/capabilities/GLInfo_1058.html.
>  The value 40 comes from 8 texture coordinates plus two colors times 4
> components.
> 

I checked the docs, and this is correct.  r300 supports 8 tex coords + 4 colors, so it should be able to do 40.  I'll see if I can make this happen.
Comment 11 Pavel Ondračka 2012-02-11 07:24:00 UTC
Is this bug related to the glsl-max-varyings piglit regression on r300g since 1ded658ce074a85bc08c989ff17840b840ff3051, or should I open a new bug?
Comment 12 Matt Turner 2013-01-30 01:54:55 UTC
I think this is fixed, at least on i965, now that Paul's packed varying series is complete. It passes here.
Comment 13 Paul Berry 2013-01-30 02:51:59 UTC
(In reply to comment #12)
> I think this is fixed, at least on i965, now that Paul's packed varying
> series is complete. It passes here.

Note that there are three separate (and IMHO unrelated) bugs being talked about here.

The original bug report was a piglit failure on i965 that happened because Mesa didn't pack varyings.  That bug was fixed by commit ca7e891e8aceaf1adb4c9ae776916fa3636747ee (glsl/linker: Pack between varyings.)

Comments 7-10 were a failure in civ4 on r300, caused by the civ4 shader using more varying floats than r300 advertises.  Mesa doesn't pack varyings on r300, and even if it did, there's no way this particular shader would be helped by varying packing.  So this is an unrelated bug, not a duplicate as we originally suspected.  Comment #10 suggests that this bug might be fixed now, and I don't have r300 hardware to test it.  If it isn't fixed, please reopen bug 43121.

In response to comment #11, the glsl-max-varyings piglit regression on r300g is definitely unrelated to this bug.  I suspect it is also unrelated to bug 43121.  I don't know if it is still present.  If it is, I would recommend opening a fresh bug report for it.

Marking this bug as fixed, since the originally reported issue has been fixed, and the issues in comments 7-11 need to be addressed in separate bug reports.
Comment 14 Gordon Jin 2013-03-06 02:08:41 UTC
Ian, can you commit the attached piglit test case (if not yet)?

Xun, please help verify.
Comment 15 Gordon Jin 2013-10-11 04:59:37 UTC
Ian?
Xun?
Comment 16 fangxun 2014-07-18 02:56:38 UTC
Verified on latest mesa master and 10.2 branch.


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.