On 23/1/19 11:27 am, Caio Marcelo de Oliveira Filho wrote:
Hi,
Did you look at any of the HURT? The problem I was seeing was this could end
up stopping copy propagation from working on some UBOs etc.
They were not UBO cases like yours, but looking at them I've found a
different problem.
However with this patch we end up with:
load UBO at offset offset_a;
if (offset_a == 10) {
load UBO at offset 10;
...
}
copy prop no longer recognizes that we are reusing the same UBO and reloads
it again.
Note that: when/if we keep derefs for UBOs longer enough to let
nir_opt_copy_prop_vars act on them, this won't be a problem as we
perform those propagations. I'm not sure you are doing the same,
though.
See also
https://gitlab.freedesktop.org/jekstrand/mesa/commits/wip/nir-ssbo-opt
-- in particular the last few patches.
+static bool
+opt_for_known_values(nir_builder *b, nir_if *nif)
+{
+ bool progress = false;
Turns out we need this one...
+ assert(nif->condition.is_ssa);
+ nir_ssa_def *if_cond = nif->condition.ssa;
+
+ if (if_cond->parent_instr->type != nir_instr_type_alu)
+ return false;
+
+ nir_alu_instr *alu = nir_instr_as_alu(if_cond->parent_instr);
+ switch (alu->op) {
+ case nir_op_feq:
+ case nir_op_ieq: {
+ nir_load_const_instr *load_const = NULL;
+ nir_ssa_def *unknown_val = NULL;
+
+ nir_ssa_def *src0 = alu->src[0].src.ssa;
+ nir_ssa_def *src1 = alu->src[1].src.ssa;
+ if (src0a->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src0->parent_instr);
+ unknown_val = src1;
+ } else if (src1->parent_instr->type == nir_instr_type_load_const) {
+ load_const = nir_instr_as_load_const(src1->parent_instr);
+ unknown_val = src0;
+ }
+
+ if (!load_const)
+ return false;
+
+ /* TODO: remove this and support swizzles? */
+ if (unknown_val->num_components != 1)
+ return false;
+
+ /* Replace unknown ssa uses with the known constant */
+ nir_foreach_use_safe(use_src, unknown_val) {
+ nir_cursor cursor = nir_before_src(use_src, false);
+ nir_block *use_block = nir_cursor_current_block(cursor);
+ if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+ nir_instr_rewrite_src(use_src->parent_instr, use_src,
+ nir_src_for_ssa(&load_const->def));
+ return true;
...but we are returning too early here. This should be "progress =
true;" instead of the return, otherwise we just replace a single use.
Then fix the final return to "return progress;".
Ah yes. Good catch.
The different problem I've found was that uses in the phi instruction
after the then/else blocks was being replaced, causing churn in the
optimizations further on. As a hack, I've ignored phi instructions in
the use loop above. HURTs are gone and HELPs continued.
The real issue seems to be use_block not being what we expect when we
have a use_src from a phi instruction. For example, in the snippet
below
vec1 1 ssa_2543 = ieq ssa_7429, ssa_6
/* succs: block_9 block_10 */
if ssa_2543 {
block block_9:
/* preds: block_8 */
vec2 32 ssa_2550 = vec2 ssa_7744, ssa_7745
vec4 32 ssa_2553 = txl ssa_468 (texture_deref), ssa_468
(sampler_deref), ssa_2550 (coord), ssa_6 (lod),
vec1 32 ssa_7434 = imov ssa_2553.w
/* succs: block_11 */
} else {
block block_10:
/* preds: block_8 */
/* succs: block_11 */
}
block block_11:
/* preds: block_9 block_10 */
vec1 32 ssa_7433 = phi block_9: ssa_7420, block_10: ssa_7420
vec1 32 ssa_7436 = phi block_9: ssa_7434, block_10: ssa_7423
vec1 32 ssa_7439 = phi block_9: ssa_7426, block_10: ssa_7426
vec1 32 ssa_7442 = phi block_9: ssa_7429, block_10: ssa_7429
the use_src that is part of the instruction "... ssa_7442 = phi ...",
was being considered use_block == 9 (the 'then'-block), so replace was
happening.
I'm not sure I see the problem, replacing the phi src is valid. The
question is why is it causing HURT?
Unless I'm missing something, replacing nir_before_src /
nir_cursor_current_block with use_src->parent_instr->block is what we
want here.
Caio
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
