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

[Lionel Landwerlin]

On 05/09/2018 13:19, Jason Ekstrand wrote:
> 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


Thanks for confirming on IRC.


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>


>
>>
>> -
>> 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
>
>
>
>