Bug 83463 - [swrast] piglit glsl-vs-clamp-1 regression
Summary: [swrast] piglit glsl-vs-clamp-1 regression
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Abdiel Janulgue
QA Contact:
URL:
Whiteboard:
Keywords: bisected, regression
: 83384 (view as bug list)
Depends on:
Blocks: 83384
  Show dependency treegraph
 
Reported: 2014-09-03 23:00 UTC by Vinson Lee
Modified: 2016-07-09 00:10 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
swrast fix (2.82 KB, patch)
2014-09-08 13:18 UTC, Abdiel Janulgue
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Vinson Lee 2014-09-03 23:00:38 UTC
mesa: 58b386dce435d2d82c2dc80b1a8d1373bb3e0ac6 (master 10.4.0-devel)

piglit glsl-vs-clamp-1 times out on swrast.

8f890b119eaff88a7fad64abbf183cbcc22edc7a is the first bad commit
commit 8f890b119eaff88a7fad64abbf183cbcc22edc7a
Author: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
Date:   Thu Jun 19 22:15:14 2014 -0700

    glsl: Optimize clamp(x, 0, 1) as saturate(x)
    
    v2: - Check that the base type is float (Ian Romanick)
    v3: - Make sure comments reflect that we are doing a commutative operation
        - Add missing condition where the inner constant is 1.0 and outer constant is 0.0
        - Make indexing of operands easier to read (Matt Turner)
    
    Reviewed-by: Matt Turner <mattst88@gmail.com>
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
    Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>

:040000 040000 5174a30266aa05b4334c831e1cd51de1376e4b70 916e9faa0afc6c04e530af2a3c0dfb938a7f07de M	src
bisect run success
Comment 1 Abdiel Janulgue 2014-09-08 12:57:28 UTC
We got an infinite loop in swrast which is caused by the back and forth ping-pong between saturate-to-clamp lowering pass and the clamp-to-saturate opt_algebraic optimization pass. I got a fix coming up which optionally disables the opt_algebraic optimization by adding an EmitNoSat toggle in gl_shader_compiler_options.
Comment 2 Abdiel Janulgue 2014-09-08 13:18:46 UTC
Created attachment 105892 [details] [review]
swrast fix

Vinson, can you please try the attached patch if it fixes the issue?
Comment 3 Kenneth Graunke 2014-09-11 05:31:05 UTC
swrast should be able to handle ir_unop_saturate just fine - it shouldn't need the lowering pass at all, even in the vertex shader.

This may still be necessary though - I don't know if r200 can do this (or if it even supports GLSL), or about pre-SM3 gallium drivers.

It sounds like drivers should set options->EmitNoSat for the VS stages, and we should only add the SAT_TO_CLAMP flag when options->EmitNoSat is set (in addition to disabling the opt_algebraic optimization in that case).  That way, saturate actually happens for drivers that can support it.
Comment 4 Vinson Lee 2014-10-11 05:39:06 UTC
attachment 105892 [details] [review] fixes swrast regression.

Tested-by: Vinson Lee <vlee@freedesktop.org>
Comment 5 Vinson Lee 2014-10-24 03:55:56 UTC
These other piglit regressions were also introduced with commit 8f890b119eaff88a7fad64abbf183cbcc22edc7a.

glsl-vs-raytrace-bug26691 
vs-smoothstep-float-float-float
vs-smoothstep-float-float-vec2 
vs-smoothstep-float-float-vec3 
vs-smoothstep-float-float-vec4 
vs-smoothstep-vec2-vec2-vec2
vs-smoothstep-vec3-vec3-vec3
vs-smoothstep-vec4-vec4-vec4 
fs-bool-less-compare-true 
vs-saturate-exp2 
vs-saturate-pow 
vs-saturate-sqrt
Comment 6 Abdiel Janulgue 2014-11-28 09:28:57 UTC
(In reply to Kenneth Graunke from comment #3)
> swrast should be able to handle ir_unop_saturate just fine - it shouldn't
> need the lowering pass at all, even in the vertex shader.
> 
> This may still be necessary though - I don't know if r200 can do this (or if
> it even supports GLSL), or about pre-SM3 gallium drivers.
> 
> It sounds like drivers should set options->EmitNoSat for the VS stages, and
> we should only add the SAT_TO_CLAMP flag when options->EmitNoSat is set (in
> addition to disabling the opt_algebraic optimization in that case).  That
> way, saturate actually happens for drivers that can support it.

Hi Ken! Sorry for the delay. I was not yet sure which drivers you mean. Do you mean *all* drivers (including r200)? Or if I understood this correctly, I would have to set EmitNoSat in the gallium state tracker and in ir_to_mesa as is done in this patch?
Comment 7 Abdiel Janulgue 2014-11-28 09:30:20 UTC
*** Bug 83384 has been marked as a duplicate of this bug. ***
Comment 8 Abdiel Janulgue 2014-12-15 13:45:05 UTC
Patch has now landed in 10.4:

http://cgit.freedesktop.org/mesa/mesa/commit/?h=10.4&id=65f03e673310fe1cdf1416d30f14a7f3edc2cd4e


bug/show.html.tmpl processed on Jan 16, 2017 at 17:18:06.
(provided by the Example extension).