On Wed, Oct 28, 2015 at 3:40 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Wed, Oct 28, 2015 at 10:37 AM, Rob Clark <robdcl...@gmail.com> wrote:
>> On Wed, Oct 28, 2015 at 12:09 PM, Jason Ekstrand <ja...@jlekstrand.net>
>> wrote:
>>> On Sat, Oct 24, 2015 at 10:08 AM, Rob Clark <robdcl...@gmail.com> wrote:
>>>> From: Rob Clark <robcl...@freedesktop.org>
>>>>
>>>> Convenient place to put in some extra sanity checking, without making
>>>> things messy for the drivers running the passes.
>>>
>>> In the short-term this seems to work (at least for testing nir_clone).
>>> In the long-term, I'm not sure that a macro is really what we want.
>>> I've mentioned a time or two before that what I *think* I'd like to do
>>> (don't know exactly how it will work out yet) is to have a little
>>> datastructure
>>>
>>> typedef struct nir_pass {
>>> bool (*shader_pass_func)(nir_shader *shader, void *data);
>>> bool (*impl_pass_func)(nir_function_impl *impl, void *data);
>>> nir_metadata metadata_preserved;
>>> void *data;
>>> } nir_pass;
>>>
>>> and have each of the passes expose one of these as a const global
>>> variable instead of exposing the actual functions. Then we would have
>>> a runner function (or macro) that could run a pass. The runner would
>>> take care of validation, trashing metadata, and maybe even cloning.
>>> If no shader_pass_func is provided but you call it on a shader, the
>>> runner would iterate over all of the overloads for you and run the
>>> impl_pass_func on each. We could also have helpers that take an array
>>> and run all of them or even take an array and run it in a loop until
>>> no more progress is made.
>>
>> meh, once we collapse the run+validate into a single line macro call,
>> having list of calls sounds like it doesn't really take up more lines
>> of code compared to a table of nir passes.. plus old fashioned code
>> has a lot more flexibility without having to reinvent loops and ifs
>> and that sort of thing. Keep in mind some passes are conditional on
>> draw state (ie. what we are lowering) or shader stage, etc.
>
> Really, colapsing run+validate is only part of the desire there. The
> bigger thing is that it moves metadata invalidation out of the passes.
> This makes it much harder to forget (I found 3 or 4 passes that do it
> wrong today).
not a bad thing.. but could also just be a nir_pass_helper type thing,
I suppose..
> This really doesn't change what you can do. If you look at my branch
> (which I'm about to send out), the only real difference between
> NIR_PASS() and nir_shader_run_pass is that one is longer and
> lower-case. You can't wholesale replace the shader, but that seems
> like something we only care about for testing cloning.
I think it is important to have testibility for clone built-in for
debug builds, and easy to enable w/out hacks/recompile. And the macro
approach is nice for dealing with stages that require extra args, so
we get better validate/clone coverage.
BR,
-R
> The thing I haven't quite settled on is how to pass extra parameters.
>>> For some passes, we could just put the extra stuff in compiler_options
>>> but we don't want to litter it too bad. The other option is to do
>>> what I did above and use the classic void pointer. Then drivers would
>>> have to just make a copy and set the data pointer to whatever they
>>> want.
>>>
>>> Maybe I should just go implement this...
>>>
>>>> TODO: convert ir3/vc4..
>>>>
>>>> Signed-off-by: Rob Clark <robcl...@freedesktop.org>
>>>> ---
>>>> src/glsl/nir/nir.h | 33 ++++++++++
>>>> src/mesa/drivers/dri/i965/brw_nir.c | 126
>>>> ++++++++++++------------------------
>>>> 2 files changed, 76 insertions(+), 83 deletions(-)
>>>>
>>>> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>>>> index 3ab720b..053420d 100644
>>>> --- a/src/glsl/nir/nir.h
>>>> +++ b/src/glsl/nir/nir.h
>>>> @@ -1939,10 +1939,43 @@ nir_shader_mutable(nir_shader *shader)
>>>>
>>>> #ifdef DEBUG
>>>> void nir_validate_shader(nir_shader *shader);
>>>> +static inline bool
>>>> +__nir_test_clone(void)
>>>> +{
>>>> + static int enable = -1;
>>>> + if (enable == -1) {
>>>> + enable = (getenv("NIR_TEST_CLONE") == NULL) ? 0 : 1;
>>>> + }
>>>> + return enable == 1;
>>>> +}
>>>> #else
>>>> static inline void nir_validate_shader(nir_shader *shader) { (void)
>>>> shader; }
>>>> +static inline bool __nir_test_clone(void) { return false; }
>>>> #endif /* DEBUG */
>>>>
>>>> +
>>>> +#define NIR_PASS(pass, nir, ...) ({ \
>>>> + assert(nir_shader_is_mutable(nir)); \
>>>> + pass(nir, ##__VA_ARGS__); \
>>>> + nir_validate_shader(nir); \
>>>> + if (__nir_test_clone()) { \
>>>> + nir = nir_shader_clone(ralloc_parent(nir), nir); \
>>>> + nir_validate_shader(nir); \
>>>> + } \
>>>> + })
>>>> +
>>>> +#define NIR_PASS_PROGRESS(pass, nir, ...) ({ \
>>>> + assert(nir_shader_is_mutable(nir)); \
>>>> + bool __ret = pass(nir, ##__VA_ARGS__); \
>>>> + nir_validate_shader(nir); \
>>>> + if (__nir_test_clone()) { \
>>>> + nir = nir_shader_clone(ralloc_parent(nir), nir); \
>>>> + nir_validate_shader(nir); \
>>>> + } \
>>>> + __ret; \
>>>> + })
>>>> +
>>>> +
>>>> void nir_calc_dominance_impl(nir_function_impl *impl);
>>>> void nir_calc_dominance(nir_shader *shader);
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_nir.c
>>>> b/src/mesa/drivers/dri/i965/brw_nir.c
>>>> index 8f09165..bc42fea 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_nir.c
>>>> +++ b/src/mesa/drivers/dri/i965/brw_nir.c
>>>> @@ -136,47 +136,34 @@ brw_nir_lower_outputs(nir_shader *nir, bool
>>>> is_scalar)
>>>> }
>>>> }
>>>>
>>>> -static void
>>>> +static nir_shader *
>>>> nir_optimize(nir_shader *nir, bool is_scalar)
>>>> {
>>>> bool progress;
>>>> do {
>>>> progress = false;
>>>> - nir_lower_vars_to_ssa(nir);
>>>> - nir_validate_shader(nir);
>>>>
>>>> - if (is_scalar) {
>>>> - nir_lower_alu_to_scalar(nir);
>>>> - nir_validate_shader(nir);
>>>> - }
>>>> + NIR_PASS(nir_lower_vars_to_ssa, nir);
>>>>
>>>> - progress |= nir_copy_prop(nir);
>>>> - nir_validate_shader(nir);
>>>> + if (is_scalar)
>>>> + NIR_PASS(nir_lower_alu_to_scalar, nir);
>>>>
>>>> - if (is_scalar) {
>>>> - nir_lower_phis_to_scalar(nir);
>>>> - nir_validate_shader(nir);
>>>> - }
>>>> + progress |= NIR_PASS_PROGRESS(nir_copy_prop, nir);
>>>> +
>>>> + if (is_scalar)
>>>> + NIR_PASS(nir_lower_phis_to_scalar, nir);
>>>>
>>>> - progress |= nir_copy_prop(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_dce(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_cse(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_peephole_select(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_algebraic(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_constant_folding(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_dead_cf(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_remove_phis(nir);
>>>> - nir_validate_shader(nir);
>>>> - progress |= nir_opt_undef(nir);
>>>> - nir_validate_shader(nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_copy_prop, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_dce, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_cse, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_peephole_select, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_algebraic, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_constant_folding, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_dead_cf, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_remove_phis, nir);
>>>> + progress |= NIR_PASS_PROGRESS(nir_opt_undef, nir);
>>>> } while (progress);
>>>> + return nir;
>>>> }
>>>>
>>>> nir_shader *
>>>> @@ -204,75 +191,52 @@ brw_create_nir(struct brw_context *brw,
>>>> }
>>>> nir_validate_shader(nir);
>>>>
>>>> - if (stage == MESA_SHADER_GEOMETRY) {
>>>> - nir_lower_gs_intrinsics(nir);
>>>> - nir_validate_shader(nir);
>>>> - }
>>>> + if (stage == MESA_SHADER_GEOMETRY)
>>>> + NIR_PASS(nir_lower_gs_intrinsics, nir);
>>>>
>>>> - nir_lower_global_vars_to_local(nir);
>>>> - nir_validate_shader(nir);
>>>> -
>>>> - nir_lower_tex(nir, &tex_options);
>>>> - nir_validate_shader(nir);
>>>> -
>>>> - nir_normalize_cubemap_coords(nir);
>>>> - nir_validate_shader(nir);
>>>> -
>>>> - nir_split_var_copies(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_lower_global_vars_to_local, nir);
>>>> + NIR_PASS(nir_lower_tex, nir, &tex_options);
>>>> + NIR_PASS(nir_normalize_cubemap_coords, nir);
>>>> + NIR_PASS(nir_split_var_copies, nir);
>>>>
>>>> - nir_optimize(nir, is_scalar);
>>>> + nir = nir_optimize(nir, is_scalar);
>>>>
>>>> /* Lower a bunch of stuff */
>>>> - nir_lower_var_copies(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_lower_var_copies, nir);
>>>>
>>>> /* Get rid of split copies */
>>>> - nir_optimize(nir, is_scalar);
>>>> + nir = nir_optimize(nir, is_scalar);
>>>>
>>>> brw_nir_lower_inputs(nir, is_scalar);
>>>> brw_nir_lower_outputs(nir, is_scalar);
>>>> nir_assign_var_locations(&nir->uniforms,
>>>> &nir->num_uniforms,
>>>> is_scalar ? type_size_scalar :
>>>> type_size_vec4);
>>>> - nir_lower_io(nir, nir_var_all,
>>>> + NIR_PASS(nir_lower_io, nir, nir_var_all,
>>>> is_scalar ? type_size_scalar : type_size_vec4);
>>>> - nir_validate_shader(nir);
>>>> -
>>>> - nir_remove_dead_variables(nir);
>>>> - nir_validate_shader(nir);
>>>>
>>>> - if (shader_prog) {
>>>> - nir_lower_samplers(nir, shader_prog);
>>>> - nir_validate_shader(nir);
>>>> - }
>>>> + NIR_PASS(nir_remove_dead_variables, nir);
>>>>
>>>> - nir_lower_system_values(nir);
>>>> - nir_validate_shader(nir);
>>>> + if (shader_prog)
>>>> + NIR_PASS(nir_lower_samplers, nir, shader_prog);
>>>>
>>>> - nir_lower_atomics(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_lower_system_values, nir);
>>>> + NIR_PASS(nir_lower_atomics, nir);
>>>>
>>>> - nir_optimize(nir, is_scalar);
>>>> + nir = nir_optimize(nir, is_scalar);
>>>>
>>>> if (brw->gen >= 6) {
>>>> /* Try and fuse multiply-adds */
>>>> - nir_opt_peephole_ffma(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_opt_peephole_ffma, nir);
>>>> }
>>>>
>>>> - nir_opt_algebraic_late(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_opt_algebraic_late, nir);
>>>>
>>>> - nir_lower_locals_to_regs(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_lower_locals_to_regs, nir);
>>>>
>>>> - nir_lower_to_source_mods(nir);
>>>> - nir_validate_shader(nir);
>>>> - nir_copy_prop(nir);
>>>> - nir_validate_shader(nir);
>>>> - nir_opt_dce(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_lower_to_source_mods, nir);
>>>> + NIR_PASS(nir_copy_prop, nir);
>>>> + NIR_PASS(nir_opt_dce, nir);
>>>>
>>>> if (unlikely(debug_enabled)) {
>>>> /* Re-index SSA defs so we print more sensible numbers. */
>>>> @@ -286,15 +250,11 @@ brw_create_nir(struct brw_context *brw,
>>>> nir_print_shader(nir, stderr);
>>>> }
>>>>
>>>> - nir_convert_from_ssa(nir, true);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_convert_from_ssa, nir, true);
>>>>
>>>> if (!is_scalar) {
>>>> - nir_move_vec_src_uses_to_dest(nir);
>>>> - nir_validate_shader(nir);
>>>> -
>>>> - nir_lower_vec_to_movs(nir);
>>>> - nir_validate_shader(nir);
>>>> + NIR_PASS(nir_move_vec_src_uses_to_dest, nir);
>>>> + NIR_PASS(nir_lower_vec_to_movs, nir);
>>>> }
>>>>
>>>> /* This is the last pass we run before we start emitting stuff. It
>>>> --
>>>> 2.5.0
>>>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
