[Mesa-dev] [PATCH v2 5/7] i965: Add a NIR analysis pass for determining when a boolean resolve is needed
Jason Ekstrand
jason at jlekstrand.net
Fri Mar 20 14:14:22 PDT 2015
On Fri, Mar 20, 2015 at 1:26 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Fri, Mar 20, 2015 at 11:24 AM, Jason Ekstrand <jason at 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.
I've added piles of comments. Will send a v2.
> 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?
No. Any bitwise operations should purely be for the purpose of
masking off the bottom 2 bits of the pass_flags. That way we can
safely use the other 6 bits of pass_flags for something else if we
want. Any other use of bitwise operations is a bug.
> 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 <jason at 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.
Yup
>> + * 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"?
Yeah, I've changed the entire thing to consistently use "resolve status"
--Jason
More information about the mesa-dev
mailing list