Bug 97309 - piglit.spec.glsl-1_30.compiler.switch-statement.switch-case-duplicated.vert regression
Summary: piglit.spec.glsl-1_30.compiler.switch-statement.switch-case-duplicated.vert r...
Status: VERIFIED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965 (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: Tapani Pälli
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords:
: 97326 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-08-11 21:41 UTC by Mark Janes
Modified: 2016-08-22 16:30 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Mark Janes 2016-08-11 21:41:39 UTC
Bisected to:
mesa ee02a5e330151630be871decde563ddbf192465c
prog_hash_table: Convert to using util/hash_table.h.

Improves glretrace -b servo.trace (a trace of Mozilla's servo rendering
engine booting, rendering a page, and exiting) from 1.8s to 1.1s.  It uses
a large uniform array of structs, making a huge number of separate program
resources, and the fixed-size hash table was killing it.  Given how many
times we've improved performance by swapping the hash table to
util/hash_table.h, just do it once and for all.

This just rebases the old hash table API on top of util/, for minimal
diff.  Cleaning things up is left for later, particularly because I want
to fix up the new hash table API a little bit.

v2: Add UNUSED to the now-unused parameter.

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>


Standard Output

/tmp/build_root/m64/lib/piglit/bin/glslparsertest /tmp/build_root/m64/lib/piglit/tests/spec/glsl-1.30/compiler/switch-statement/switch-case-duplicated.vert fail 1.30
Shader source:
// [config]
// expect_result: fail
// glsl_version: 1.30
// [end config]
//
// From page 57 (page 63 of the PDF) of the GLSL 1.30 spec:
//
//     "It is an error to have more than one default or a replicated
//      constant-expression."

#version 130

void main() {
   int tmp = 0;
   switch (1) {
   case 0:
   case 0:
      tmp = 1;
   }

   gl_Position = vec4(0.0);
}

Standard Error

Successfully compiled vertex shader /tmp/build_root/m64/lib/piglit/tests/spec/glsl-1.30/compiler/switch-statement/switch-case-duplicated.vert:
Comment 1 Mark Janes 2016-08-12 18:29:34 UTC
*** Bug 97326 has been marked as a duplicate of this bug. ***
Comment 2 Tapani Pälli 2016-08-17 10:21:26 UTC
The problem here is they way switch label hashtable is built, it uses case label as the key and in the test this key is '0'. Now when new hash table searches entries it has a check 'entry_is_present' that checks if key != NULL and this fails in the particular case where case label is 0. One easy way is just to store key + 1 to avoid using 0, I'll investigate if there would be nicer way.
Comment 3 Tapani Pälli 2016-08-17 10:30:45 UTC
> One easy way is just to store key + 1 to avoid using 0

Which would obviously fail when value is -1 :) Anyway, basic idea would be to avoid using 0 as key.
Comment 4 Tapani Pälli 2016-08-17 11:23:10 UTC
I believe I have a working fix (uses _mesa_hash_data for key), will run testing and send to mesa-dev if no regressions.
Comment 5 Tapani Pälli 2016-08-22 06:07:15 UTC
fixed by:
--- 8< ---
commit 68233801aeb73961cd47dbba276e5d6fcf5411fc
Author: Tapani Pälli <tapani.palli@intel.com>
Date:   Fri Aug 19 13:44:54 2016 +0300

    glsl: fix key used for hashing switch statement cases
    
    Implementation previously used value itself as the key, however after
    hash implementation change by ee02a5e we cannot use 0 as key.
    
    v2: use constant pointer as the key and implement comparison
        for contents (Eric Anholt)
    
    Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
    Reviewed-by: Eric Anholt <eric@anholt.net>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97309


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.