On Wed, Mar 18, 2015 at 6:07 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
> Overall, I think it might be better to do this as a SSA pass right
> before lowering to source modifiers. I don't think we would be running
> any optimizations after it that depend on the output being all 0's or
> all 1's, but if you're concerned about that then we could add separate
> gen5-compare compare opcodes and lower the normal ones to those plus
> any necessary resolves.
Sure, we could do that. However, the logic required is the same either way.
> On Tue, Mar 17, 2015 at 10:17 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>> ---
>> src/mesa/drivers/dri/i965/Makefile.sources | 2 +
>> src/mesa/drivers/dri/i965/brw_nir.h | 45 ++++
>> .../dri/i965/brw_nir_analize_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_analize_boolean_resolves.c
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
>> b/src/mesa/drivers/dri/i965/Makefile.sources
>> index c69441b..05ebbe9 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_analize_boolean_resolves.c \
>
> I must say, this is a rather unfortunate typo/spelling mistake... I
> always read it as anal-ize and then my inner 5 year old comes out and
> I giggle. To be honest, I couldn't even get through the entire thing
> because of this. You're not the first person to do it though.
Sorry... I'll fix that. /me and spelling...
>> 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..f79c5f1
>> --- /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
>> +
>> +#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,
>> + BRW_NIR_BOOLEAN_NEEDS_RESOLVE = 0x1,
>> + BRW_NIR_BOOLEAN_UNRESOLVED = 0x2,
>> + BRW_NIR_BOOLEAN_NO_RESOLVE = 0x3,
>> + BRW_NIR_BOOLEAN_MASK = 0x3,
>> +};
>> +
>> +void brw_nir_analize_boolean_resolves(nir_shader *nir);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif
>> diff --git a/src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c
>> b/src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c
>> new file mode 100644
>> index 0000000..7d93471
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_nir_analize_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
>> + * 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
>> + * 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
>> +analize_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:
>> + /* For ALU instructions, we handle [un]resolved booleans below. */
>
> Might it be better to put the code for handling ALU instructions here
> instead? In effect we're only running it for ALU instructions since
> the rest of the cases end in a continue, so why not make that more
> obvious by just moving it up here under the ALU case?
Sure. That's fine with me.
>> + break;
>> +
>> + case nir_instr_type_load_const: {
>> + /* For load_const instructions, it's a boolean exactly when it
>> holds
>> + * one of the values NIR_TRUE or NIR_FALSE.
>> + */
>> + nir_load_const_instr *load = nir_instr_as_load_const(instr);
>> + if (load->value.u[0] == NIR_TRUE || load->value.u[0] == NIR_FALSE)
>> {
>> + instr->pass_flags |= BRW_NIR_BOOLEAN_NO_RESOLVE;
>> + } else {
>> + instr->pass_flags |= BRW_NIR_NON_BOOLEAN;
>> + }
>> + continue;
>> + }
>> +
>> + default:
>> + /* Everything else is an unknown non-boolean value and needs to
>> + * have all sources resolved.
>> + */
>> + instr->pass_flags |= BRW_NIR_NON_BOOLEAN;
>> + nir_foreach_src(instr, src_mark_needs_resolve, NULL);
>> + continue;
>> + }
>> +
>> + uint8_t bool_status;
>> + nir_alu_instr *alu = nir_instr_as_alu(instr);
>> + switch (alu->op) {
>> + case nir_op_flt:
>> + case nir_op_ilt:
>> + case nir_op_ult:
>> + case nir_op_fge:
>> + case nir_op_ige:
>> + case nir_op_uge:
>> + case nir_op_feq:
>> + case nir_op_ieq:
>> + case nir_op_fne:
>> + case nir_op_ine:
>> + case nir_op_f2b:
>> + case nir_op_i2b:
>> + bool_status = BRW_NIR_BOOLEAN_UNRESOLVED;
>> +
>> + /* Even though the destination is allowed to be left unresolved,
>> + * we need to resolve all the sources of a compare.
>> + */
>> + nir_foreach_src(instr, src_mark_needs_resolve, NULL);
>> + break;
>> +
>> + case nir_op_imov:
>> + case nir_op_inot:
>> + if (alu->dest.write_mask == 1) {
>> + /* This is a single-source instruction. Just copy the
>> resolution
>> + * state from the source.
>> + */
>> + bool_status = get_resolve_state_for_src(alu, 0);
>> + } else {
>> + bool_status = BRW_NIR_NON_BOOLEAN;
>> + }
>> + break;
>> +
>> + case nir_op_iand:
>> + case nir_op_ior:
>> + case nir_op_ixor: {
>> + assert(alu->dest.write_mask == 1);
>> +
>> + uint8_t src0_flags = get_resolve_state_for_src(alu, 0);
>> + uint8_t src1_flags = get_resolve_state_for_src(alu, 1);
>> +
>> + if (src0_flags == src1_flags) {
>> + bool_status = src0_flags;
>> + } else if (src0_flags == BRW_NIR_NON_BOOLEAN ||
>> + src1_flags == BRW_NIR_NON_BOOLEAN) {
>> + bool_status = BRW_NIR_NON_BOOLEAN;
>> + } else {
>> + /* At this point one of them is a true boolean and one is a
>> + * boolean that needs a resolve. We could either resolve the
>> + * unresolved source or we could resolve here. If we resolve
>> + * the unresolved source then we get two resolves for the price
>> + * of one. Just set this one to BOOLEAN_NO_RESOLVE and we'll
>> + * let the code below force a resolve on the unresolved source.
>> + */
>> + bool_status = BRW_NIR_BOOLEAN_NO_RESOLVE;
>> + }
>> + break;
>> + }
>> +
>> + default:
>> + bool_status = BRW_NIR_NON_BOOLEAN;
>> + }
>> +
>> + /* If the destination is SSA-like, go ahead allow unresolved booleans.
>> + * If the destination register doesn't have a well-defined
>> parent_instr
>> + * we need to resolve immediately.
>> + */
>> + if (alu->dest.dest.reg.reg->parent_instr == NULL &&
>> + bool_status == BRW_NIR_BOOLEAN_UNRESOLVED) {
>> + bool_status = BRW_NIR_BOOLEAN_NEEDS_RESOLVE;
>> + }
>> +
>> + instr->pass_flags |= bool_status;
>> +
>> + /* Finally, resolve sources if it's needed */
>> + switch (bool_status) {
>> + case BRW_NIR_BOOLEAN_NEEDS_RESOLVE:
>> + case BRW_NIR_BOOLEAN_UNRESOLVED:
>> + /* This instruction is either unresolved or we're doing the
>> + * resolve here; leave the sources alone.
>> + */
>> + break;
>> +
>> + case BRW_NIR_BOOLEAN_NO_RESOLVE:
>> + case BRW_NIR_NON_BOOLEAN:
>> + nir_foreach_src(instr, src_mark_needs_resolve, NULL);
>> + break;
>> +
>> + default:
>> + unreachable("Invalid boolean flag");
>> + }
>> + }
>> +
>> + nir_if *following_if = nir_block_get_following_if(block);
>> + if (following_if)
>> + src_mark_needs_resolve(&following_if->condition, NULL);
>> +
>> + return true;
>> +}
>> +
>> +static void
>> +analize_boolean_resolves_impl(nir_function_impl *impl)
>> +{
>> + nir_foreach_block(impl, analize_boolean_resolves_block, NULL);
>> +}
>> +
>> +void
>> +brw_nir_analize_boolean_resolves(nir_shader *shader)
>> +{
>> + nir_foreach_overload(shader, overload)
>> + if (overload->impl)
>> + analize_boolean_resolves_impl(overload->impl);
>> +}
>> --
>> 2.3.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
