Bug 92822 - arb_program_interface_query.arb_program_interface_query-getprogramresourceindex regression
Summary: arb_program_interface_query.arb_program_interface_query-getprogramresourceind...
Status: RESOLVED MOVED
Alias: None
Product: Mesa
Classification: Unclassified
Component: glsl-compiler (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: low normal
Assignee: Timothy Arceri
QA Contact: Intel 3D Bugs Mailing List
URL:
Whiteboard:
Keywords: bisected, regression
: 107639 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-11-04 17:55 UTC by Mark Janes
Modified: 2019-09-18 19:45 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Mark Janes 2015-11-04 17:55:26 UTC
When Arrays of Arrays was enabled, the following piglit test regressed on snb, bdw, bsw, byt, hsw, ivb, and skl:

piglit.spec.arb_program_interface_query.arb_program_interface_query-getprogramresourceindex


/tmp/build_root/m64/lib/piglit/bin/arb_program_interface_query-getprogramresourceindex -auto
piglit: debug: Requested an OpenGL 3.2 Core Context, and received a matching 3.3 context

Invalid index for 'vs_input2': expected INVALID_INDEX but got 0
Invalid index for 'vs_input2[1][0]': expected INVALID_INDEX but got 0

The first CI build with this failure was 6e3b380387378e9f8e92eed3dc4a95767857b0de:
Author:     Timothy Arceri <t_arceri@yahoo.com.au>
AuthorDate: Fri Oct 16 10:28:48 2015 +1100
Commit:     Timothy Arceri <t_arceri@yahoo.com.au>
CommitDate: Wed Nov 4 13:41:16 2015 +1100

    docs: Mark AoA as done for i965
    
    Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

However, it may be one of the two previous patches that triggers the regression.

In addition, the following piglit tests were enabled with AoA, but fail:

piglit.spec.glsl-1_10.execution.varying-packing.simple uvec3 arrays_of_arrays
piglit.spec.glsl-1_10.execution.varying-packing.simple ivec3 arrays_of_arrays

Standard Output:
Probe color at (0,0)
  Expected: 0.000000 1.000000 0.000000 1.000000
  Observed: 1.000000 1.000000 1.000000 1.000000

Standard Error

Failed to link: error: fragment shader uses too many input components (132 > 128)
Comment 1 Mark Janes 2015-11-04 19:57:05 UTC
Tim has reported that the following (sub)tests are new, not regressions:

piglit.spec.glsl-1_10.execution.varying-packing.simple uvec3 arrays_of_arrays
piglit.spec.glsl-1_10.execution.varying-packing.simple ivec3 arrays_of_arrays
piglit.spec.arb_program_interface_query.arb_program_interface_query-getprogramresourceindex

However, subsequent testing has shown that the following tests are *intermittent* on BDW only:

arb_arrays_of_arrays.execution.image_store.basic-imagestore-non-const-uniform-index
arb_arrays_of_arrays.execution.image_store.basic-imagestore-const-uniform-index


These tests are disabled for that platform until this bug is resolved.
Comment 2 Timothy Arceri 2015-11-23 03:50:41 UTC
1/3 problems reported in this bug report are now fixed.

Fixed:

piglit.spec.glsl-1_10.execution.varying-packing.simple uvec3 arrays_of_arrays
piglit.spec.glsl-1_10.execution.varying-packing.simple ivec3 arrays_of_arrays

This was a problem with the piglit test and has been fixed.
Comment 3 Mark Janes 2015-12-04 21:48:54 UTC
<tarceri> [13:30:26] janesma, I still have 92822 on my list but as I said to
          Ian here when he marked it as a blocker for 11.1 its not a
          regression
<tarceri> [13:30:55] I don't see why it should be a blocker for the 11.1
          release
<tarceri> [13:34:14] There are two issues 1. a bug with image store on bdw
          which also happens on the non arrays of arrays tests 2. A bug with
          arb_program_interface_query for arrays of arrays which again is not
          a regression just a bug
<tarceri> [13:34:27] with new functionality 
<tarceri> [13:36:37] Unfortunately the fix for 2. is not as simple as it
          seems, issue 1. I have been unable to reproduce but is clearly a bug
          with image store rather than AoA
Comment 4 Francisco Jerez 2015-12-09 13:48:01 UTC
The following intermittent failures are very likely to be bugs in the piglit tests rather than in Mesa:

 arb_arrays_of_arrays.execution.image_store.basic-imagestore-non-const-uniform-index
 arb_arrays_of_arrays.execution.image_store.basic-imagestore-const-uniform-index

Neither of them runs glMemoryBarrier() before reading from the image they render to, so their behaviour is unspecified.

I haven't looked into the ARB_program_interface_query bug in detail, which is likely unrelated, but the above failures shouldn't be considered a blocker for the Mesa 11.1 release.
Comment 5 Timothy Arceri 2015-12-10 00:35:20 UTC
(In reply to Francisco Jerez from comment #4)
> The following intermittent failures are very likely to be bugs in the piglit
> tests rather than in Mesa:
> 
>  arb_arrays_of_arrays.execution.image_store.basic-imagestore-non-const-
> uniform-index
>  arb_arrays_of_arrays.execution.image_store.basic-imagestore-const-uniform-
> index
> 
> Neither of them runs glMemoryBarrier() before reading from the image they
> render to, so their behaviour is unspecified.
> 

Thanks for taking a look at this. I've sent a piglit series to try fix this here:

http://patchwork.freedesktop.org/series/1663/

If this looks correct then I will need to push and get Mark to check that this resolves the issue as I can't reproduce it on my BDW.
Comment 6 Mark Janes 2015-12-10 20:49:38 UTC
The patch seems to have resolved the issue with spec.arb_arrays_of_arrays.execution.image_store.basic-imagestore-non-const-uniform-index
Comment 7 Timothy Arceri 2015-12-13 03:35:40 UTC
(In reply to Mark Janes from comment #6)
> The patch seems to have resolved the issue with
> spec.arb_arrays_of_arrays.execution.image_store.basic-imagestore-non-const-
> uniform-index

Thanks for testing Mark.

I have pushed these patches to the piglit repo so the issue with these tests should be fixed as of feeefb1b47b758.

That makes 2/3 issues being tracked in this bug fixed.

Only piglit.spec.arb_program_interface_query.arb_program_interface_query-getprogramresourceindex remains to be fixed.

This one requires a Mesa fix, there are two options:

1.
Create a lowering pass the turns all arrays of arrays into seperate single dimensions arrays.

For example an AoA:  
in my_aoa[2][2][2];

Would become:
in my_aoa[0][0][2];
in my_aoa[0][1][2];
in my_aoa[1][0][2];
in my_aoa[1][1][2];

Where the outer two subscripts are now just part of the variable name rather than being functional. This will also allow inactive arrays to be easily found.

However it will be a bit of work to get this correct as indirects will need to be handled somehow as well as things such as assignments of whole AoA.
Also a bunch of clean-up would be needed as its a major change in the way AoA are currently handled after the initial compile/linking validation is complete.

2.
The other simpler option would be to process the existing IR and store the values we need for arb_program_interface_query in a newly allocated piece of memory used just for interface querys.
Comment 8 Mark Janes 2016-06-15 21:55:47 UTC
removing release tracker references.

Based on the commentary in this bug, the remaining piglit failure is a new test, not a regression.  This bug should not block releases.
Comment 9 asimiklit 2018-08-21 13:03:21 UTC
*** Bug 107639 has been marked as a duplicate of this bug. ***
Comment 10 asimiklit 2018-08-31 14:20:05 UTC
Hi all,

The solution for the last issue is suggested:
https://patchwork.freedesktop.org/patch/246791/

and test extensions which helped me to find some issues in my patch:
https://patchwork.freedesktop.org/patch/246789/

Regards,
Andrii.
Comment 11 Timothy Arceri 2019-02-15 23:12:57 UTC
Just to update this bug report. I ended up suggesting against the solution proposed by Andrii because it was too much code for something that didn't actually remove the unused components, but just hid them from the resource list.

I think if we actually want to fix this properly then we could do it by finally hooking up a NIR linker for GLSL.

For example after the linking (i.e. when we produce the resource list) the GLSL IR is converted to this NIR form:

shader: MESA_SHADER_VERTEX
name: GLSL3
inputs: 0
outputs: 0
uniforms: 0
shared: 0
decl_var shader_in INTERP_MODE_NONE vec4[2][2] vs_input2 (VERT_ATTRIB_GENERIC8, 0, 0)
decl_var shader_in INTERP_MODE_NONE vec4[2][2][2] vs_input3 (VERT_ATTRIB_GENERIC0, 0, 0)
decl_var shader_out INTERP_MODE_NONE vec4 gl_Position (VARYING_SLOT_POS, 0, 0)
decl_function main (0 params)

impl main {
	block block_0:
	/* preds: */
	vec1 32 ssa_0 = deref_var &vs_input2 (shader_in vec4[2][2]) 
	vec1 32 ssa_1 = load_const (0x00000000 /* 0.000000 */)
	vec1 32 ssa_2 = deref_array &(*ssa_0)[0] (shader_in vec4[2]) /* &vs_input2[0] */
	vec1 32 ssa_3 = deref_array &(*ssa_2)[0] (shader_in vec4) /* &vs_input2[0][0] */
	vec4 32 ssa_4 = intrinsic load_deref (ssa_3) (0) /* access=0 */
	vec1 32 ssa_5 = deref_var &vs_input3 (shader_in vec4[2][2][2]) 
	vec1 32 ssa_6 = deref_array &(*ssa_5)[0] (shader_in vec4[2][2]) /* &vs_input3[0] */
	vec1 32 ssa_7 = deref_array &(*ssa_6)[0] (shader_in vec4[2]) /* &vs_input3[0][0] */
	vec1 32 ssa_8 = deref_array &(*ssa_7)[0] (shader_in vec4) /* &vs_input3[0][0][0] */
	vec4 32 ssa_9 = intrinsic load_deref (ssa_8) (0) /* access=0 */
	vec1 32 ssa_10 = fadd ssa_4.x, ssa_9.x
	vec1 32 ssa_11 = fadd ssa_4.y, ssa_9.y
	vec1 32 ssa_12 = fadd ssa_4.z, ssa_9.z
	vec1 32 ssa_13 = fadd ssa_4.w, ssa_9.w
	vec1 32 ssa_14 = deref_var &gl_Position (shader_out vec4) 
	vec4 32 ssa_15 = vec4 ssa_10, ssa_11, ssa_12, ssa_13
	intrinsic store_deref (ssa_14, ssa_15) (15, 0) /* wrmask=xyzw */ /* access=0 */
	/* succs: block_1 */
	block block_1:
}


But due to better NIR having better optimisations this ends up as:

shader: MESA_SHADER_VERTEX
name: GLSL3
inputs: 2
outputs: 1
uniforms: 0
shared: 0
decl_var shader_in INTERP_MODE_NONE vec4 vs_input2 (VERT_ATTRIB_GENERIC8, 4, 0)
decl_var shader_in INTERP_MODE_NONE vec4 vs_input3 (VERT_ATTRIB_GENERIC0, 0, 0)
decl_var shader_out INTERP_MODE_NONE vec4 gl_Position (VARYING_SLOT_POS, 0, 0)
decl_function main (0 params)

impl main {
	block block_0:
	/* preds: */
	vec1 32 ssa_0 = deref_var &vs_input2 (shader_in vec4) 
	vec4 32 ssa_1 = intrinsic load_deref (ssa_0) (0) /* access=0 */
	vec1 32 ssa_2 = deref_var &vs_input3 (shader_in vec4) 
	vec4 32 ssa_3 = intrinsic load_deref (ssa_2) (0) /* access=0 */
	vec1 32 ssa_4 = fadd ssa_1.x, ssa_3.x
	vec1 32 ssa_5 = fadd ssa_1.y, ssa_3.y
	vec1 32 ssa_6 = fadd ssa_1.z, ssa_3.z
	vec1 32 ssa_7 = fadd ssa_1.w, ssa_3.w
	vec1 32 ssa_8 = deref_var &gl_Position (shader_out vec4) 
	vec4 32 ssa_9 = vec4 ssa_4, ssa_5, ssa_6, ssa_7
	intrinsic store_deref (ssa_8, ssa_9) (15, 0) /* wrmask=xyzw */ /* access=0 */
	/* succs: block_1 */
	block block_1:
}


If we were to build the resource list from a NIR linker this piglit test would pass this test just like the Nvidia blob.
Comment 12 GitLab Migration User 2019-09-18 19:45:33 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/807.


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.