On Fri, Mar 20, 2015 at 11:24 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> v2: Fix the spelling of analyze and re-arrange code for better readability
> as per Connor's comments.
> ---
> src/mesa/drivers/dri/i965/Makefile.sources | 2 +
> src/mesa/drivers/dri/i965/brw_nir.h | 45 ++++
> .../dri/i965/brw_nir_analyze_boolean_resolves.c | 228
> +++++++++++++++++++++
> 3 files changed, 275 insertions(+)
> create mode 100644 src/mesa/drivers/dri/i965/brw_nir.h
> create mode 100644
> src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
> b/src/mesa/drivers/dri/i965/Makefile.sources
> index c69441b..3a3df70 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.sources
> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> @@ -73,6 +73,8 @@ i965_FILES = \
> brw_meta_util.h \
> brw_misc_state.c \
> brw_multisample_state.h \
> + brw_nir.h \
> + brw_nir_analyze_boolean_resolves.c \
> brw_object_purgeable.c \
> brw_packed_float.c \
> brw_performance_monitor.c \
> diff --git a/src/mesa/drivers/dri/i965/brw_nir.h
> b/src/mesa/drivers/dri/i965/brw_nir.h
> new file mode 100644
> index 0000000..4a81647
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_nir.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#pragma once
I think we decided that we still need actual include guards.
> +
> +#include "glsl/nir/nir.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/* Flags set in the instr->pass_flags field by i965 analysis passes */
> +enum {
> + BRW_NIR_NON_BOOLEAN = 0x0,
Meaning is obvious.
> + BRW_NIR_BOOLEAN_NEEDS_RESOLVE = 0x1,
We decided this instruction's destination needs to be resolved?
> + BRW_NIR_BOOLEAN_UNRESOLVED = 0x2,
This is different from BRW_NIR_BOOLEAN_NEEDS_RESOLVE in
cmp res1 ...
cmp res2 ...
and res3 res2 res1
in that res1 and res2 are BRW_NIR_BOOLEAN_UNRESOLVED while res3 is
BRW_NIR_BOOLEAN_NEEDS_RESOLVE?
> + BRW_NIR_BOOLEAN_NO_RESOLVE = 0x3,
I'm not sure what this one means.
We're doing bit operations below on these enum values, so it's
surprising to see that
BRW_NIR_BOOLEAN_NEEDS_RESOLVE | BRW_NIR_BOOLEAN_UNRESOLVED ==
BRW_NIR_BOOLEAN_NO_RESOLVE
Is that intentional?
All in all, some comments explaining what these enums mean would help
the reader significantly.
> + BRW_NIR_BOOLEAN_MASK = 0x3,
> +};
> +
> +void brw_nir_analyze_boolean_resolves(nir_shader *nir);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> diff --git a/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c
> b/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c
> new file mode 100644
> index 0000000..b19d80b
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c
> @@ -0,0 +1,228 @@
> +/*
> + * Copyright © 2015 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + * Jason Ekstrand <ja...@jlekstrand.net>
> + */
> +
> +#include "brw_nir.h"
> +
> +/*
> + * This file implements an analysis pass that determines when we have to do
> + * a boolean resolve on GEN <= 5. Instructions that need a boolean resolve
It doesn't stand for anything, so we just write Gen.
> + * will have the booleans portion of the instr->pass_flags field set to
> + * BRW_NIR_BOOLEAN_NEEDS_RESOLVE.
> + */
> +
> +static uint8_t
> +get_resolve_state_for_src(nir_alu_instr *alu, unsigned src_idx)
> +{
> + nir_instr *src_instr;
> + if (alu->src[src_idx].src.is_ssa) {
> + src_instr = alu->src[src_idx].src.ssa->parent_instr;
> + } else {
> + src_instr = alu->src[src_idx].src.reg.reg->parent_instr;
> + }
> +
> + if (src_instr) {
> + uint8_t state = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK;
> +
> + /* If the source instruction nees resolve then, from the perspective
typo: needs
I had a hard time parsing this sentence until I mentally moved the
comma from after 'then' to before it.
> + * of the user, it's a true boolean.
> + */
> + if (state == BRW_NIR_BOOLEAN_NEEDS_RESOLVE)
> + state = BRW_NIR_BOOLEAN_NO_RESOLVE;
> + return state;
> + } else {
> + return BRW_NIR_NON_BOOLEAN;
> + }
> +}
> +
> +static bool
> +src_mark_needs_resolve(nir_src *src, void *void_state)
> +{
> + if (src->is_ssa)
> + return true;
> +
> + if (src->reg.reg->parent_instr == NULL)
> + return true;
> +
> + nir_instr *src_instr = src->reg.reg->parent_instr;
> +
> + uint8_t bool_flags = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK;
> + if (bool_flags == BRW_NIR_BOOLEAN_UNRESOLVED) {
> + src_instr->pass_flags &= ~BRW_NIR_BOOLEAN_MASK;
> + src_instr->pass_flags |= BRW_NIR_BOOLEAN_NEEDS_RESOLVE;
> + }
> +
> + return true;
> +}
> +
> +static bool
> +analyze_boolean_resolves_block(nir_block *block, void *void_state)
> +{
> + nir_foreach_instr(block, instr) {
> + /* Clear the boolean state */
> + instr->pass_flags &= ~BRW_NIR_BOOLEAN_MASK;
> +
> + switch (instr->type) {
> + case nir_instr_type_alu: {
> + uint8_t bool_status;
Reading uses of this variable is difficult... "bool status equals ..."
Change it to "resolve_status"?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
