Created attachment 115410 [details]
Test case for shader_runner

It looks like nir_lower_samplers calculates the wrong sampler number when the sampler uniform is in an array. Here's a simpler example for shader_runner which demonstrates the problem.

Created attachment 115411 [details]
Test case for shader_runner

Oops, here's a better version of that test.

Created attachment 115527 [details] [review]
workaround for the issue

Here's a workaround. I believe the calculation code expects that we have array of samplers and does not understand the situation where we have arrays of structures with samplers in them. I haven't quite understood from the code why this is so, will continue investigation. Attached workaround fixes this issue.

So, is this a bug, or a test issue?

Tested it on BDW and SNB with the latest nightly kernel(2ead07) and Mesa(1ac7db07b), this problem has gone away.
So Verified.

(In reply to ye.tian from comment #5)
> Tested it on BDW and SNB with the latest nightly kernel(2ead07) and
> Mesa(1ac7db07b), this problem has gone away.
> So Verified.

I find this hard to believe, could you still verify again? I can still reproduce this on HSW with current Mesa head (b5045e2).

(In reply to Tapani Pälli from comment #6)
> (In reply to ye.tian from comment #5)
> > Tested it on BDW and SNB with the latest nightly kernel(2ead07) and
> > Mesa(1ac7db07b), this problem has gone away.
> > So Verified.
> 
> I find this hard to believe, could you still verify again? I can still
> reproduce this on HSW with current Mesa head (b5045e2).

It still fails on the latest mesa master branch, Fixed by comment 3's patch.
Reopen it until the patch merged.

(In reply to Gavin Hindman from comment #4)
> So, is this a bug, or a test issue?

This is a bug in Mesa, I've received some comments on the patch and will try to improve it, it won't work for all the cases as is.

Created attachment 115599 [details]
more complex shader_runner test

This test has additional cases with array of samplers and array of structures containing array of samplers.

Created attachment 115601 [details] [review]
fix for the issue

here's another try for the fix, for me this passes all of piglit and the attached 'complex' case

(In reply to Tapani Pälli from comment #10)
> Created attachment 115601 [details] [review] [review]
> fix for the issue
> 
> here's another try for the fix, for me this passes all of piglit and the
> attached 'complex' case

I'm still not convinced that's correct, but it made me look through the code again and I think I know what's going wrong.  On line 81 in the direct array access case we increment sampler_index by deref->base_offset *and* add "[%u]" to the string description with deref->base_offset.  Then sampler_index is incremented by the return value of get_sampler_index which should take the array offset into account *again*.  Maybe we just need to remove the "instr->sampler_index += deref->base_offset" line to get rid of the double-counting.

(In reply to Tapani Pälli from comment #10)
> Created attachment 115601 [details] [review] [review]
> fix for the issue
> 
> here's another try for the fix, for me this passes all of piglit and the
> attached 'complex' case

Fixed by this patch.

(In reply to Jason Ekstrand from comment #11)
> (In reply to Tapani Pälli from comment #10)
> > Created attachment 115601 [details] [review] [review] [review]
> > fix for the issue
> > 
> > here's another try for the fix, for me this passes all of piglit and the
> > attached 'complex' case
> 
> I'm still not convinced that's correct, but it made me look through the code
> again and I think I know what's going wrong.  On line 81 in the direct array
> access case we increment sampler_index by deref->base_offset *and* add
> "[%u]" to the string description with deref->base_offset.  Then
> sampler_index is incremented by the return value of get_sampler_index which
> should take the array offset into account *again*.  Maybe we just need to
> remove the "instr->sampler_index += deref->base_offset" line to get rid of
> the double-counting.

Ah right, that's what the first (workaround patch) causes and the second patch too but by accident. It looks to me that deref_array->base_offset is required *only* in the case of having array of samplers (), otherwise it should be not taken to account. I will test this and set a patch.

sending another try to mesa-dev

fix pushed to master

Verified.fixed.

(In reply to Tapani Pälli from comment #15)
> fix pushed to master

commit 95774ca258d216d42877f9a8da7e1bb4212a6500
Author: Tapani Pälli <tapani.palli@intel.com>
Date:   Mon May 11 14:50:19 2015 +0300

    nir: fix sampler lowering pass for arrays

(please include this from now on)