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

[Jason Ekstrand]

On Sat, Nov 7, 2015 at 5:21 AM, Rob Clark <robdclark at gmail.com> wrote:
> 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..

We could restructure things to make nir_compiler_options something we
create more on-the-fly but, as it currently stands, we create 1 or 2
of them total for the life of the driver.

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

Like I said above, go ahead and push it.  We can sort out the issues
once we have multiple drivers using it.  Trying to sort it out now is
kind of theoretical at best.