Bug 84225 - Allow constant-index-expression sampler array indexing with GLSL-ES < 300
Summary: Allow constant-index-expression sampler array indexing with GLSL-ES < 300
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Tapani Pälli
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-23 06:55 UTC by kalyank
Modified: 2015-06-30 08:33 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix (1.13 KB, text/plain)
2014-09-23 06:55 UTC, kalyank
Details
attempt to fix this issue (3.89 KB, patch)
2015-04-01 08:22 UTC, Tapani Pälli
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description kalyank 2014-09-23 06:55:59 UTC
Created attachment 106707 [details]
Fix

Restrictions were added in GLSL 1.30 to allow indexing of arrays with
integral constants only. Earlier versions (GLSL <= 1.20 and GLSL-ES <=100)
should allow any sort of sampler array indexing (i.e. Non constant expressions).

Mesa already has this in place but seems the checks fail in case of GLSL-ES ver being 100.

Proposed patch
Comment 1 Ian Romanick 2014-09-24 01:12:53 UTC
This is not required in ESSL 1.00.  Section 10.24 (Samplers) of the ESSL 1.00 spec says:

    Should samplers be allowed as l-values? The specification already
    allows an equivalent behavior:

    Current specification:

        uniform sampler2D sampler[8];
        int index = f(...);
        vec4 tex = texture2D(sampler[index], xy); // allowed

    Using assignment of sampler types:

        uniform sampler2D s;
        s = g(...);
        vec4 tex = texture2D(s, xy); // not allowed

    RESOLUTION: Keep current specification. Support for dynamic
    indexing of arrays of samplers is not mandated for ES2.0
    (specified in the limitations section).

Appendix A (Limitations for ES 2.0), subsecton 5 (Indexing of Arrays, Vectors and Matrices), paragraph "Samplers" (oh good grief!) says:

    GLSL ES 1.00 supports both arrays of samplers and arrays of
    structures which contain samplers. In both these cases, for ES 2.0,
    support for indexing with a constant-index-expression is mandated
    but support for indexing with other values is not mandated.

Mesa supports OpenGL ES 2.0 on some hardware (e.g., i915) that cannot support dynamic indexing of sampler arrays.  I believe we are conformant as-is.
Comment 2 kalyank 2014-09-24 06:19:16 UTC
Thx for pointing this out.
Comment 3 Tapani Pälli 2015-03-27 06:15:07 UTC
What is missing seems to be the "constant-index-expression" part (meaning indexing with an expression that includes loop induction variable). GLSL ES spec states (table on page 110) that "constant-index-expression" must be supported for sampler arrays. I will try to build a test for this behaviour.
Comment 4 Tapani Pälli 2015-03-27 06:50:29 UTC
I've sent a glslparser test here:
http://lists.freedesktop.org/archives/piglit/2015-March/015360.html
Comment 5 Tapani Pälli 2015-03-27 08:16:36 UTC
Bug can be also reproduced with following demo when running Chrome with GLES backend:

https://www.modern.ie/en-us/demos/fishgl
Comment 6 Tapani Pälli 2015-04-01 07:23:50 UTC
I will try to tackle this.
Comment 7 Tapani Pälli 2015-04-01 08:22:41 UTC
Created attachment 114800 [details] [review]
attempt to fix this issue

Attached is an WIP attempt to fix the issue. It fixes the Piglit test I've sent, unfortunately does not fix the fishgl which is more complex as it uses clamp function inside the index (thus introducing a temporary inside index).
Comment 8 Tapani Pälli 2015-04-01 11:12:09 UTC
Some more braindump; it looks like it would make sense to move array index validation further down, now we do it in AST->HIR before running optimizations and lowering passes that can potentially change the index expression in to a legal form of indexing. IMO this happens with the fishgl demo.
Comment 9 Tapani Pälli 2015-04-01 12:18:51 UTC
We've had some discussion about this issue with Curro. It would make sense to make a small patch like Kalyan's to just enabled dynamic indexing and then (if absolutely necessary) some additional validation added later during compilation/linking.

The minimal change would be to just change error to warning with:

--- 8< --------------------------------
if (!state->is_version(130, 300)) {
}
--- 8< --------------------------------

This way we don't need to write potentially a lot of complicate code just because of one silly app.
Comment 10 Tapani Pälli 2015-04-07 09:53:30 UTC
FYI, I sent a v2 of the 'Fix' to mesa-dev mailing list.
Comment 11 Tapani Pälli 2015-04-14 08:31:34 UTC
update: I'm still working on this (as a background task), this has taken long time because of unexpected problems.  When adding a check that array index is a constant after compilation I noticed that loops (sometimes or maybe always) get unrolled only during linking which makes me think maybe there are some issues with loop analysis pass, we should be able to unroll a loop during compilation. With my Piglit test unrolling gets called 3 times during compilation but only 4th time (happens during linking) will unroll loop which in order makes the array index constant ... will continue investigation.
Comment 12 Tapani Pälli 2015-05-19 05:07:49 UTC
according to Kalyan there are 2 more cases hitting this issue:

http://hexgl.bkcore.com/play
http://users.tpg.com.au/rubixcom/index.html
Comment 13 Tapani Pälli 2015-05-19 12:01:48 UTC
I've sent another Piglit test and 2 patches to Mesa mailing list that fix this.
Comment 14 Tapani Pälli 2015-05-26 05:24:00 UTC
(In reply to Tapani Pälli from comment #13)
> I've sent another Piglit test and 2 patches to Mesa mailing list that fix
> this.

Sorry, this did not go 'like in Strömsö'. I'm working on another proposal with changes the loop analysis so that we unroll sooner and can make the required checks at compile time, this should go according to language spec.
Comment 15 Tapani Pälli 2015-05-27 08:01:16 UTC
I've also noticed current WebGL 2.0 conformance tests require this feature (although it is not in GLSL ES 3.0), I've filed a Khronos bug to discuss this issue.
Comment 16 Tapani Pälli 2015-06-09 10:58:07 UTC
here's a branch with new patches to enable this:

http://cgit.freedesktop.org/~tpalli/mesa/log/?h=unroll_loops
Comment 17 Gavin Hindman 2015-06-23 22:13:07 UTC
What's the status on this issue?
Comment 18 Tapani Pälli 2015-06-24 10:41:30 UTC
(In reply to Gavin Hindman from comment #17)
> What's the status on this issue?

I have received feedback on the patches and rebased + corrected issues found. Updated patches are on top of tree here:

http://cgit.freedesktop.org/~tpalli/mesa/log/?h=unroll_loops

I did not receive r-b for one of the patches so I will try to resolve that, It seems I will need to add support for the added option also for i915 and gallium drivers which will be couple of additional patches.
Comment 19 Tapani Pälli 2015-06-30 08:33:30 UTC
fix pushed to master + planned for 10.5 and 10.6 branches


bug/show.html.tmpl processed on Feb 24, 2017 at 03:43:49.
(provided by the Example extension).