[Mesa-dev] [PATCH 6/7] nir: fix missing increments of num_inputs/num_outputs

[Rob Clark]

On Fri, Nov 6, 2015 at 10:20 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Fri, Nov 6, 2015 at 5:10 PM, Rob Clark <robdclark at gmail.com> wrote:
>> On Fri, Nov 6, 2015 at 6:39 PM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Fri, Nov 6, 2015 at 6:23 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>> On Fri, Nov 6, 2015 at 8:35 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>>
>>>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>>>> ---
>>>>>  src/glsl/nir/nir_lower_clip.c            | 2 ++
>>>>>  src/glsl/nir/nir_lower_two_sided_color.c | 2 ++
>>>>>  2 files changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/src/glsl/nir/nir_lower_clip.c b/src/glsl/nir/nir_lower_clip.c
>>>>> index 31ccfb2..4a91527 100644
>>>>> --- a/src/glsl/nir/nir_lower_clip.c
>>>>> +++ b/src/glsl/nir/nir_lower_clip.c
>>>>> @@ -55,9 +55,11 @@ create_clipdist_var(nir_shader *shader, unsigned drvloc,
>>>>>
>>>>>     if (output) {
>>>>>        exec_list_push_tail(&shader->outputs, &var->node);
>>>>> +      shader->num_outputs++;
>>>>>     }
>>>>>     else {
>>>>>        exec_list_push_tail(&shader->inputs, &var->node);
>>>>> +      shader->num_inputs++;
>>>>
>>>> I'm not sure what I think about this.  Usually, num_inputs/outputs is
>>>> set by nir_lower_io or similar.  They don't have to be vec4's.  In the
>>>> i965 driver, FS inputs are in terms of floats.  Maybe tgsi_to_nir
>>>> provides you those guarantees but we haven't for i965.
>>>
>>> hmm, what do you recommend then?  There isn't really any
>>> straightforward way to run this *prior* to lower_io... tgsi->nir gives
>>> me something that is already i/o lowered, and what I'm doing so far w/
>>> gallium support for glsl->nir is doing lower_io (and a few other
>>> steps) in gallium so that the result the driver gets is equivalent to
>>> tgsi->nir..
>
> Hrm... That does make things a bit sticky.  If you'd like, you can go
> ahead and push it with
>
> Akced-by: Jason Ekstrand <jason.ekstrand at intel.com>
>
>> btw Jason, how do you feel about stuffing the type_size fxn ptr in
>> nir_shader_compiler_options?  It shouldn't ever change over the
>> lifetime of the shader, we could drop it as arg to
>> nir_assign_var_locations() (err, well, replace w/ nir_shader ptr), and
>> we could use it to dtrt here..
>
> That won't work.  The type_size function we use depends on shader
> stage, gen, and type of thing we're lowering.  Stuffing it into
> nir_shader_compiler_options won't work.

hmm, but the type_size should be invariant over the lifetime of the
nir_shader object, shouldn't it?  (Well, maybe we need to be able to
specify different one's for uniform/input/output?)  I guess if you
don't already have different compiler_options's for scalar vs simd
modes, then they could be passed in to nir_shader_create() instead..

The other option is just to pass type_size into these other lowering
passes and use that instead of +=1..  but seems like type_size should
be constant for a given nir_shader, so not having to pass them around
all over the place seems nice.

BR,
-R