On Wed, Mar 18, 2015 at 9:10 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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.
Well, you wouldn't have to worry about registers with no parent_instr,
so it would be simpler. And you wouldn't need backend-specific pass
flags that rely on other passes not stomping on them, so it would be a
little cleaner too.
>
>> 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
