On Mon, Feb 8, 2016 at 2:00 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Mon, Feb 8, 2016 at 1:55 PM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Mon, Feb 8, 2016 at 1:01 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
>>> On Mon, Feb 8, 2016 at 8:59 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>>> On Mon, Feb 8, 2016 at 6:58 AM, Roland Scheidegger <srol...@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 <robdcl...@gmail.com> wrote:
>>>>>>> On Sun, Jan 31, 2016 at 3:16 PM, Rob Clark <robdcl...@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.
If you really have enough diff settings between gen's then just embed
the struct in your pipe_screen and populate it at screen init..
Currently we've managed to keep nir_shader_compiler_options as
something that can be const (by keeping options that depend on draw
state out). It would be annoying to loose that. Not to mention that
it would be duplication for the NIR drivers, since they already need
the nir options sturct in tgsi_to_nir case.
>>
>> 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;
> }
> }
meh, common stuff would be duplication from what is already in nir 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...
yeah, new stuff in options struct seems sane
BR,
-R
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
