[Mesa-dev] [RFCv3 11/11] mesa/st: add support for NIR as possible driver IR

[Ilia Mirkin]

On Mon, Feb 8, 2016 at 1:55 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Mon, Feb 8, 2016 at 1:01 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Mon, Feb 8, 2016 at 8:59 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Mon, Feb 8, 2016 at 6:58 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>> Am 06.02.2016 um 22:30 schrieb Marek Olšák:
>>>>> On Sat, Feb 6, 2016 at 2:45 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>> On Sun, Jan 31, 2016 at 3:16 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>> +   // XXX get from pipe_screen?  Or just let pipe driver provide?
>>>>>>> +   nir_options.lower_fpow = true;
>>>>>>> +   nir_options.lower_fsat = true;
>>>>>>> +   nir_options.lower_scmp = true;
>>>>>>> +   nir_options.lower_flrp = true;
>>>>>>> +   nir_options.lower_ffract = true;
>>>>>>> +   nir_options.native_integers = true;
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> btw, one of the few remaining things to tackle is how to handle
>>>>>> nir_shader_compiler_options struct.  To follow the existing approach
>>>>>> of shader caps, I'd have to add a big pile of caps now, and then keep
>>>>>> adding them as nir_shader_compiler_options struct grows.  Which seems
>>>>>> sub-optimal.
>>>>>>
>>>>>> How do people feel about adding a screen->get_shader_paramp() which,
>>>>>> along the lines of get_paramf, returns a 'const void *'?  Then we
>>>>>> could add a single cap to return the whole compiler-options struct.
>>>>>> (And maybe if at some point there was direct support for LLVM as an
>>>>>> IR, it might need something similar??)
>>>>>>
>>>>>> Other possibility is just a pipe->get_nir_compiler_options() type
>>>>>> hook.  A bit more of a point solution, but might make sense if we
>>>>>> can't think of any other plausible uses for ->get_shader_paramp()..
>>>>>> and less churn since it would only need to be implemented by drivers
>>>>>> consuming NIR..
>>>>>>
>>>>>> Thoughts/opinions?
>>>>>
>>>>> pipe->get_nir_compiler_options() sounds good.
>>>>>
>>>>> Maybe wait for VMWare guys' opinion as well.
>>>>
>>>> Looks usable to me, albeit I'm not sure you really need NIR-specific
>>>> options as such? That is those options above don't really look nir
>>>> specific - maybe they aren't used with just glsl->tgsi, but it looks to
>>>> me like they would in theory be applicable to other IR as well. Though I
>>>> suppose if you just had compiler_otions it would be a bit confusing if
>>>> you had entries which then may not be used...
>>>
>>> Yeah, not really NIR specific (and there are a couple that overlap w/
>>> existing caps), other than being used only by NIR..  although it would
>>> be a lot of churn to keep adding caps when the compiler_options struct
>>> is extended, and it might be confusing that some of the lowering
>>> options aren't supported in the TGSI path..
>>>
>>> I guess right now it really only matters for two drivers, and down the
>>> road I think we won't have more than 3 or 4 drivers using NIR, so I
>>> suppose it is also an option to start w/
>>> screen->get_nir_compiler_options() for now and revisit later.  If we
>>> get to the point where we are always doing glsl->nir and then
>>> optionally nir->tgsi for drivers that don't consume NIR directly,
>>> maybe then it would make more sense to expose everything as caps?
>>
>> I actually kinda want this for TGSI as well, eventually. Perhaps something like
>>
>> bool get_compiler_options(pipe_shader_ir, void *)
>
> perhaps:
>
>   const struct pipe_compiler_options * (*get_compiler_options)(struct
> pipe_screen *, unsigned shader)
>
> imo, it should take shader stage as arg so we can have different
> config per stage, and return a const ptr.. and I think it could
> directly return options struct (no need for bool return)..
>
> I suppose if you plan to add lots of knobs to twiddle for TGSI then
> shader cap for each would be annoying.  Although not super-thrilled
> about having to translate from generic(ish) pipe struct to nir struct,
> since the driver will already want a const version of the nir struct
> for the tgsi_to_nir case.
>
> I guess we could do:
>
>   const void * (*get_compiler_options)(struct pipe_screen *, unsigned
> shader, enum pipe_shader_ir type)

That means the driver has to allocate and store the options somewhere.
This can get annoying... I'd rather just have it fill in a struct
that's passed in.

>
> where the return value could be 'struct tgsi_shader_options *' or
> 'struct nir_shader_compiler_options *', etc..

Well I was thinking that there'd be a

struct pipe_compiler_options {
  some common stuff?
  union {
    struct nir_options;
    struct tgsi_options;
  }
}

>
> hmm.. also not sure how to roll that out without a flag day.  Perhaps
> keep the shader params for now (for tgsi) with a helper to populate a
> tgsi_compiler_options struct for drivers where
> screen->get_compiler_options() is null.. (and then what about other
> st's?)

Yeah, definitely some sort of transition plan would have to happen.
And maybe leave all the current stuff as-is, but then add new things
to compiler options? Or some other decision...

  -ilia