Bug 94297

Summary: piglit.spec.arb_tessellation_shader.compiler.barrier-switch-always.tesc fails
Product: Mesa Reporter: Mark Janes <mark.a.janes>
Component: Drivers/DRI/i965Assignee: Intel 3D Bugs Mailing List <intel-3d-bugs>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: low CC: kenneth
Version: gitKeywords: bisected, regression
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
i915 platform: i915 features:

Description Mark Janes 2016-02-25 21:43:36 UTC
Mesa 9d9aeb91b regresses this test on ivb and later:

Standard Error

Successfully compiled and linked tessellation control shader /tmp/build_root/m64/lib/piglit/tests/spec/arb_tessellation_shader/compiler/barrier-switch-always.tesc
Comment 1 Kenneth Graunke 2016-06-01 02:45:40 UTC
Although this is technically a regression, I don't really consider it critical.

The problem is that the GLSL compiler checks for barrier being in an allowed position too late.  We passed this test previously because our switch statements always generated lousy code (pointless loops that ran for a single iteration). Now, we don't.

Also, the result of this bug is that we accept programs that we should reject according to the rules.  We also happen to compile them and do what the author intended.  So it's a portability problem (apps written against our driver may work on our driver but not work on other drivers), but not going to break any valid apps.
Comment 2 Kenneth Graunke 2016-06-06 18:31:05 UTC
Removing from the 12.0 release tracker.
Comment 3 Matt Turner 2016-11-03 21:49:53 UTC
The CI system says that this commit fixed the test, which is somewhat surprising.

commit 943b69cddd2ae90e0b0fcab2dff4a7eea81bb3cc
Author: Kenneth Graunke <kenneth@whitecape.org>
Date:   Tue Nov 11 22:32:27 2014 -0800

    glsl: Delete linker stuff relating to built-in functions.

Ken, could you confirm?
Comment 4 Mark Janes 2016-11-03 22:11:11 UTC
The actual commit was earlier in the series:

glsl: Check TCS barrier restrictions at ast_to_hir time, not link time.

We want to check prior to optimization - otherwise we might fail to
detect cases where barrier() is in control flow which is always taken
(and therefore gets optimized away).

We don't currently loop unroll if there are function calls inside;
otherwise we might have a problem detecting barrier() in loops that
get unrolled as well.

Tapani's switch handling code adds a loop around switch statements, so
even with the mess of if ladders, we'll properly reject it.

Enforcing these rules at compile time makes more sense more sense than
link time.  Doing it at ast-to-hir time (rather than as an IR pass)
allows us to emit an error message with proper line numbers.
(Otherwise, I would have preferred the IR pass...)

Fixes spec/arb_tessellation_shader/compiler/barrier-switch-always.tesc.
Comment 5 Kenneth Graunke 2016-11-04 03:02:38 UTC
Mark is right.  I had to rewrite the barrier error enforcement code as part of that series - the old approach worked on ir_calls, which is incompatible with the idea of inlining all calls right off the bat.  So while I was at it, I fixed this simple bug.

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.