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

[Rob Clark]

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)

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

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

BR,
-R

> or perhaps struct pipe_compiler_options * (which would contain some
> common stuff + a union of the per-ir options) instead of the void *
> would make more sense? The reason I'm interested is to be able to
> indicate that various frontend opts should be disabled. Also it could
> be used to get rid of a bunch of PIPE_CAP_TGSI_XYZ's, which are a huge
> pain to add all the time.
>
>   -ilia