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. > > 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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev