[Mesa-dev] [PATCH] anv/pipeline: Only consider double elements which actually exist

[Jason Ekstrand]

On September 5, 2018 05:35:40 Lionel Landwerlin 
<lionel.g.landwerlin at intel.com> wrote:

> On 05/09/2018 11:24, Lionel Landwerlin wrote:
>> On 05/09/2018 00:04, Jason Ekstrand wrote:
>>> The brw_vs_prog_data::double_inputs_read field comes directly from
>>> shader_info::double_inputs which may contain inputs which are not
>>> actually read.  Instead of using it directly, AND it with inputs_read
>>> which is only things which are read.  Otherwise, we may end up
>>> subtracting too many elements when computing elem_count.
>>>
>>> Cc: mesa-stable at lists.freedesktop.org
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103241
>>
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>
>
> Actually, I would like to take that back for a minute.
>
> As far as I can tell, the content of inputs_read & double_inputs_read
> are copied directly from what gets computed in NIR.
> For VS stage, both variable seems to be set a set in lock step in
> src/compiler/nir/nir_gather_info.c
> So I would expect the following assertion to be true :
> (vs_prog_data->double_inputs_read & vs_prog_data->inputs_read) ==
> vs_prog_data->double_inputs_read
>
> Isn't the problem here that inputs_read get compacted in
> src/compiler/nir/nir_linking_helpers.c and double_inputs_read isn't
> updated accordingly.
> If that's case, this change might not actually fix the bug.

No, the problem, as stated in the commit message, is that it comes from 
double_inputs, not double_inputs_read.  But shouldn't I just change that?  
Ok, fair enough, but unfortunately doing so would break i965. My 64-bit 
attribute detangling series will help with that problem and I've got 
another patch it two in the works which will make prog_data sensible again.

--Jason

>
> -
> Lionel
>
>
>>
>>
>>> ---
>>>  src/intel/vulkan/genX_pipeline.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/intel/vulkan/genX_pipeline.c
>>> b/src/intel/vulkan/genX_pipeline.c
>>> index b531205508c..297e9455b51 100644
>>> --- a/src/intel/vulkan/genX_pipeline.c
>>> +++ b/src/intel/vulkan/genX_pipeline.c
>>> @@ -91,7 +91,8 @@ emit_vertex_input(struct anv_pipeline *pipeline,
>>>       /* Pull inputs_read out of the VS prog data */
>>>     const uint64_t inputs_read = vs_prog_data->inputs_read;
>>> -   const uint64_t double_inputs_read =
>>> vs_prog_data->double_inputs_read;
>>> +   const uint64_t double_inputs_read =
>>> +      vs_prog_data->double_inputs_read & inputs_read;
>>>     assert((inputs_read & ((1 << VERT_ATTRIB_GENERIC0) - 1)) == 0);
>>>     const uint32_t elements = inputs_read >> VERT_ATTRIB_GENERIC0;
>>>     const uint32_t elements_double = double_inputs_read >>
>>> VERT_ATTRIB_GENERIC0;
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev