Bug 109573

Summary: dEQP-VK.spirv_assembly.instruction.graphics.module.same_module
Product: Mesa Reporter: Mark Janes <mark.a.janes>
Component: Drivers/DRI/i965Assignee: Kenneth Graunke <kenneth>
Status: RESOLVED FIXED QA Contact: Intel 3D Bugs Mailing List <intel-3d-bugs>
Severity: normal    
Priority: medium Keywords: bisected, regression
Version: git   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Mark Janes 2019-02-06 18:40:12 UTC
This test regressed with:

ef99f4c8d176f4e854e12afa1545fa53f651d758
Author:     Kenneth Graunke <kenneth@whitecape.org>

compiler: Mark clip/cull distance arrays as compact before lowering.

nir_lower_clip_cull_distance_arrays() marks the combined clip/cull
distance array as compact.  However, when translating in from GLSL
or SPIR-V, we were not marking the original float[] arrays as compact.

We should do so.  That way, we can detect these corner cases properly.


-------------------------------

FWIW, this test is listed as previously fixed by
3b804819650cc943fcd4cccedd140e4b27fbf993

Patch seems unrelated, so it was possibly fixed by some patch just before it.
Comment 1 Kenneth Graunke 2019-02-07 02:40:23 UTC
I looked into this for a few hours today, it's unfortunately a bit more complicated than I'd hoped.  My patch is correct, it's just exposing a pre-existing issue.

That test has a single SPIR-V module containing multiple shader stages.  When compiling that module, we generate shader inputs and outputs for every stage...in a NIR shader targeting a particular shader stage.  So, we suddenly have VS output variables in a TCS.  The TCS expects outputs to be per-vertex arrays (or marked patch).  VS outputs are not.  So, they are illegal in a TCS.

In particular, there is a gl_PerVertex output variable for another stage in the TCS, which is being structure split, giving us top-level output for clipdist[x] and culldist[x].  But in a TCS, those would need an extra dimension - dist[x][y].  The validator tries to enforce that variables marked 'compact' must be arrays, and after unwrapping the per-vertex IO array dimension, it sees simply 'float', which is rightly illegal.

I'm testing a patch to remove dead input/output variables at the end of spirv_to_nir(), which should eliminate the invalid ones before the validator ever sees the shenanigans.  Jason recommended this, and we take a similar approach already in some cases.
Comment 2 Kenneth Graunke 2019-02-07 04:23:35 UTC
MR 215 opened: https://gitlab.freedesktop.org/mesa/mesa/merge_requests/215
Comment 3 Mark Janes 2019-02-14 21:41:15 UTC
fixed by

origin/master 6775665e5eec7db3f291508a8b7ba2a792f62ec0
Author:     Kenneth Graunke <kenneth@whitecape.org>

spirv: Eliminate dead input/output variables after translation.

spirv_to_nir can generate input/output variables which are illegal
for the current shader stage, which would cause nir_validate_shader
to balk.  After my recent commit to start decorating arrays as compact,
dEQP-VK.spirv_assembly.instruction.graphics.module.same_module started
hitting validation errors due to outputs in a TCS (not intended for the
TCS at all) not being per-vertex arrays.

Thanks to Jason Ekstrand for suggesting this approach.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109573
Fixes: ef99f4c8d17 compiler: Mark clip/cull distance arrays as compact before lowering.

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.