Re: [Mesa-dev] [PATCH v2 1/8] nir: evaluate if condition uses inside the if branches
On Fri, Aug 17, 2018 at 1:13 PM Jason Ekstrand wrote:
> On Mon, Jul 23, 2018 at 3:03 AM Timothy Arceri
> wrote:
>
>> Since we know what side of the branch we ended up on we can just
>> replace the use with a constant.
>>
>> All the spill changes in shader-db are from Dolphin uber shaders,
>> despite some small regressions the change is clearly positive.
>>
>> shader-db results IVB:
>>
>> total instructions in shared programs: 201 -> 9993483 (-0.06%)
>> instructions in affected programs: 163235 -> 157517 (-3.50%)
>> helped: 132
>> HURT: 2
>>
>> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
>> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
>> helped: 115
>> HURT: 24
>>
>> total spills in shared programs: 4383 -> 4370 (-0.30%)
>> spills in affected programs: 1656 -> 1643 (-0.79%)
>> helped: 9
>> HURT: 18
>>
>> total fills in shared programs: 4610 -> 4581 (-0.63%)
>> fills in affected programs: 374 -> 345 (-7.75%)
>> helped: 6
>> HURT: 0
>> ---
>> src/compiler/nir/nir_opt_if.c | 124 ++
>> 1 file changed, 124 insertions(+)
>>
>> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
>> index b3d0bf1decb..b3d5046a76e 100644
>> --- a/src/compiler/nir/nir_opt_if.c
>> +++ b/src/compiler/nir/nir_opt_if.c
>> @@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)
>> return true;
>> }
>>
>> +static void
>> +replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,
>> +void *mem_ctx, bool if_condition)
>> +{
>> + /* Create const */
>> + nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1,
>> 32);
>> + load->value.u32[0] = nir_boolean;
>> +
>> + if (if_condition) {
>> + nir_instr_insert_before_cf(>parent_if->cf_node,
>> >instr);
>>
>
> If it was me, I'd probably use the builder but I think it's a wash in this
> case.
>
>
>> + } else if (use->parent_instr->type == nir_instr_type_phi) {
>> + nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);
>> +
>> + bool UNUSED found = false;
>> + nir_foreach_phi_src(phi_src, cond_phi) {
>> + if (phi_src->src.ssa == use->ssa) {
>>
>
> You could also just use some sort of container_of macro to cast from the
> src to a phi_src. It's a bit sneaky so maybe not a good idea for the tiny
> bit of perf.
>
>
>> +nir_instr_insert_before_block(phi_src->pred, >instr);
>>
>
> after_block_before_jump would work just as well and would put the
> load_const closer to its use.
>
>
>> +found = true;
>> +break;
>> + }
>> + }
>> + assert(found);
>> + } else {
>> + nir_instr_insert_before(use->parent_instr, >instr);
>> + }
>> +
>> + /* Rewrite use to use const */
>> + nir_src new_src = nir_src_for_ssa(>def);
>>
>
> Is there a good reason for the temporary variable?
>
>
>> +
>> + if (if_condition)
>> + nir_if_rewrite_condition(use->parent_if, new_src);
>> + else
>> + nir_instr_rewrite_src(use->parent_instr, use, new_src);
>> +}
>>
>
> Ok, enough nitpicking. None of the above things are actually problems.
>
>
>> +
>> +static bool
>> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
>> + bool if_condition)
>> +{
>> + bool progress = false;
>> +
>> + nir_block *use_block;
>> + if (if_condition) {
>> + use_block =
>> +
>> nir_cf_node_as_block(nir_cf_node_prev(_src->parent_if->cf_node));
>> + } else {
>> + use_block = use_src->parent_instr->block;
>>
>
> Not true for phis!
>
>
>> + }
>> +
>> + if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
>> + replace_if_condition_use_with_const(use_src, NIR_TRUE, mem_ctx,
>> + if_condition);
>> + progress = true;
>> + } else if (nir_block_dominates(nir_if_first_else_block(nif),
>> use_block)) {
>> + replace_if_condition_use_with_const(use_src, NIR_FALSE, mem_ctx,
>> + if_condition);
>> + progress = true;
>> + }
>> +
>> + return progress;
>> +}
>>
>
> I think things would be more straightforward (and correct!) if you merged
> the above two functions and restructured them a bit as follows:
>
> static bool
> try_rewrite_if_use(nir_builder *b, nir_if *nif, nir_src *src, bool
> if_condition)
> {
>if (if_condition) {
> b->cursor = nir_before_cf_node(>cf_node);
>} else if (src->parent_instr->type == nir_instr_type_phi) {
> // Set the cursor and use_block to the predecessor block
>} else {
> b->cursor = nir_before_instr(src->parent_instr);
>}
>nir_block *use_block = nir_cursor_current_block(b->cursor);
>
>nir_ssa_def *const_val;
>if (nir_block_dominates(nir_if_first_then_block(nif), use_block))
> const_value = nir_imm_int(b, NIR_TRUE);
>else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)
> const_value = nir_imm_int(b,
Re: [Mesa-dev] [PATCH v2 1/8] nir: evaluate if condition uses inside the if branches
On Mon, Jul 23, 2018 at 3:03 AM Timothy Arceri
wrote:
> Since we know what side of the branch we ended up on we can just
> replace the use with a constant.
>
> All the spill changes in shader-db are from Dolphin uber shaders,
> despite some small regressions the change is clearly positive.
>
> shader-db results IVB:
>
> total instructions in shared programs: 201 -> 9993483 (-0.06%)
> instructions in affected programs: 163235 -> 157517 (-3.50%)
> helped: 132
> HURT: 2
>
> total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
> cycles in affected programs: 143424120 -> 131229457 (-8.50%)
> helped: 115
> HURT: 24
>
> total spills in shared programs: 4383 -> 4370 (-0.30%)
> spills in affected programs: 1656 -> 1643 (-0.79%)
> helped: 9
> HURT: 18
>
> total fills in shared programs: 4610 -> 4581 (-0.63%)
> fills in affected programs: 374 -> 345 (-7.75%)
> helped: 6
> HURT: 0
> ---
> src/compiler/nir/nir_opt_if.c | 124 ++
> 1 file changed, 124 insertions(+)
>
> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
> index b3d0bf1decb..b3d5046a76e 100644
> --- a/src/compiler/nir/nir_opt_if.c
> +++ b/src/compiler/nir/nir_opt_if.c
> @@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)
> return true;
> }
>
> +static void
> +replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,
> +void *mem_ctx, bool if_condition)
> +{
> + /* Create const */
> + nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1,
> 32);
> + load->value.u32[0] = nir_boolean;
> +
> + if (if_condition) {
> + nir_instr_insert_before_cf(>parent_if->cf_node, >instr);
>
If it was me, I'd probably use the builder but I think it's a wash in this
case.
> + } else if (use->parent_instr->type == nir_instr_type_phi) {
> + nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);
> +
> + bool UNUSED found = false;
> + nir_foreach_phi_src(phi_src, cond_phi) {
> + if (phi_src->src.ssa == use->ssa) {
>
You could also just use some sort of container_of macro to cast from the
src to a phi_src. It's a bit sneaky so maybe not a good idea for the tiny
bit of perf.
> +nir_instr_insert_before_block(phi_src->pred, >instr);
>
after_block_before_jump would work just as well and would put the
load_const closer to its use.
> +found = true;
> +break;
> + }
> + }
> + assert(found);
> + } else {
> + nir_instr_insert_before(use->parent_instr, >instr);
> + }
> +
> + /* Rewrite use to use const */
> + nir_src new_src = nir_src_for_ssa(>def);
>
Is there a good reason for the temporary variable?
> +
> + if (if_condition)
> + nir_if_rewrite_condition(use->parent_if, new_src);
> + else
> + nir_instr_rewrite_src(use->parent_instr, use, new_src);
> +}
>
Ok, enough nitpicking. None of the above things are actually problems.
> +
> +static bool
> +evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
> + bool if_condition)
> +{
> + bool progress = false;
> +
> + nir_block *use_block;
> + if (if_condition) {
> + use_block =
> +
> nir_cf_node_as_block(nir_cf_node_prev(_src->parent_if->cf_node));
> + } else {
> + use_block = use_src->parent_instr->block;
>
Not true for phis!
> + }
> +
> + if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
> + replace_if_condition_use_with_const(use_src, NIR_TRUE, mem_ctx,
> + if_condition);
> + progress = true;
> + } else if (nir_block_dominates(nir_if_first_else_block(nif),
> use_block)) {
> + replace_if_condition_use_with_const(use_src, NIR_FALSE, mem_ctx,
> + if_condition);
> + progress = true;
> + }
> +
> + return progress;
> +}
>
I think things would be more straightforward (and correct!) if you merged
the above two functions and restructured them a bit as follows:
static bool
try_rewrite_if_use(nir_builder *b, nir_if *nif, nir_src *src, bool
if_condition)
{
if (if_condition) {
b->cursor = nir_before_cf_node(>cf_node);
} else if (src->parent_instr->type == nir_instr_type_phi) {
// Set the cursor and use_block to the predecessor block
} else {
b->cursor = nir_before_instr(src->parent_instr);
}
nir_block *use_block = nir_cursor_current_block(b->cursor);
nir_ssa_def *const_val;
if (nir_block_dominates(nir_if_first_then_block(nif), use_block))
const_value = nir_imm_int(b, NIR_TRUE);
else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)
const_value = nir_imm_int(b, NIR_FALSE);
else
return false;
// Rewrite the source
return true;
}
Other than the phi issue I pointed out above (which would be fixed by that
refactor), everything looks good to me.
--Jason
> +
> +static bool
>
Re: [Mesa-dev] [PATCH v2 1/8] nir: evaluate if condition uses inside the if branches
On 26/07/18 14:04, Timothy Arceri wrote:
On 23/07/18 18:02, Timothy Arceri wrote:
Since we know what side of the branch we ended up on we can just
replace the use with a constant.
All the spill changes in shader-db are from Dolphin uber shaders,
despite some small regressions the change is clearly positive.
shader-db results IVB:
total instructions in shared programs: 201 -> 9993483 (-0.06%)
instructions in affected programs: 163235 -> 157517 (-3.50%)
helped: 132
HURT: 2
total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
cycles in affected programs: 143424120 -> 131229457 (-8.50%)
helped: 115
HURT: 24
total spills in shared programs: 4383 -> 4370 (-0.30%)
spills in affected programs: 1656 -> 1643 (-0.79%)
helped: 9
HURT: 18
total fills in shared programs: 4610 -> 4581 (-0.63%)
fills in affected programs: 374 -> 345 (-7.75%)
helped: 6
HURT: 0
---
src/compiler/nir/nir_opt_if.c | 124 ++
1 file changed, 124 insertions(+)
diff --git a/src/compiler/nir/nir_opt_if.c
b/src/compiler/nir/nir_opt_if.c
index b3d0bf1decb..b3d5046a76e 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)
return true;
}
+static void
+replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,
+ void *mem_ctx, bool if_condition)
+{
+ /* Create const */
+ nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx,
1, 32);
+ load->value.u32[0] = nir_boolean;
+
+ if (if_condition) {
+ nir_instr_insert_before_cf(>parent_if->cf_node,
>instr);
+ } else if (use->parent_instr->type == nir_instr_type_phi) {
+ nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);
+
+ bool UNUSED found = false;
+ nir_foreach_phi_src(phi_src, cond_phi) {
+ if (phi_src->src.ssa == use->ssa) {
+ nir_instr_insert_before_block(phi_src->pred, >instr);
I've just noticed this needs to be nir_instr_insert_after_block()
otherwise we can end up inserting the const before so other phi which
trips up validation.
I've fixed this locally.
Ok we can't do that either because the last instruction in the block can
be a jump. So I've reworked this to insert the const after the last phi.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 1/8] nir: evaluate if condition uses inside the if branches
On 23/07/18 18:02, Timothy Arceri wrote:
Since we know what side of the branch we ended up on we can just
replace the use with a constant.
All the spill changes in shader-db are from Dolphin uber shaders,
despite some small regressions the change is clearly positive.
shader-db results IVB:
total instructions in shared programs: 201 -> 9993483 (-0.06%)
instructions in affected programs: 163235 -> 157517 (-3.50%)
helped: 132
HURT: 2
total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
cycles in affected programs: 143424120 -> 131229457 (-8.50%)
helped: 115
HURT: 24
total spills in shared programs: 4383 -> 4370 (-0.30%)
spills in affected programs: 1656 -> 1643 (-0.79%)
helped: 9
HURT: 18
total fills in shared programs: 4610 -> 4581 (-0.63%)
fills in affected programs: 374 -> 345 (-7.75%)
helped: 6
HURT: 0
---
src/compiler/nir/nir_opt_if.c | 124 ++
1 file changed, 124 insertions(+)
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index b3d0bf1decb..b3d5046a76e 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)
return true;
}
+static void
+replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,
+void *mem_ctx, bool if_condition)
+{
+ /* Create const */
+ nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);
+ load->value.u32[0] = nir_boolean;
+
+ if (if_condition) {
+ nir_instr_insert_before_cf(>parent_if->cf_node, >instr);
+ } else if (use->parent_instr->type == nir_instr_type_phi) {
+ nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);
+
+ bool UNUSED found = false;
+ nir_foreach_phi_src(phi_src, cond_phi) {
+ if (phi_src->src.ssa == use->ssa) {
+nir_instr_insert_before_block(phi_src->pred, >instr);
I've just noticed this needs to be nir_instr_insert_after_block()
otherwise we can end up inserting the const before so other phi which
trips up validation.
I've fixed this locally.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
