Re: [Mesa-dev] [PATCH] intel/ir: Fix CFG corruption in opt_predicated_break().

[Matt Turner]

On Tue, Jul 23, 2019 at 5:58 PM Francisco Jerez  wrote:
>
> Specifically the optimization of a conditional BREAK + WHILE sequence
> into a conditional WHILE seems pretty broken.  The list of successors
> of "earlier_block" (where the conditional BREAK was found) is emptied
> and then re-created with the same edges for no apparent reason.  On
> top of that the list of predecessors of the block immediately after
> the WHILE loop is emptied, but only one of the original edges will be
> added back, which means that potentially several blocks that still
> have it on their list of successors won't be on its list of
> predecessors anymore, causing all sorts of hilarity due to the
> inconsistency in the control flow graph.

It's been ~5 years since I wrote the code, but I think the idea was to
prevent ::combine_with from ever combining blocks that had "internal"
edges -- that is, the first block cannot have outgoing edges and the
second block cannot have incoming edges. That still seems to me to be
a good idea. I guess it would be fine if that were relaxed to be the
only

The intention was to ensure that in the rare cases that we use
::combine_with that the programmer has to think hard about how they're
rewiring the CFG.

> The solution is to remove the code that's removing valid edges from
> the CFG.  cfg_t::remove_block() will already clean up after itself.
> The assert in bblock_t::combine_with() also needs to be removed since
> we will be merging a block with multiple children into the first one
> of them.

Okay, so I think you're saying that ::remove_block already does
exactly what we need so we don't need to bother ensuring that the
"internal" edges are removed. If that's an accurate restatement of
your claim then this patch makes sense to me and is

Reviewed-by: Matt Turner 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/ir: Fix CFG corruption in opt_predicated_break().

[Paul Chelombitko]

I've run the test from the
https://bugs.freedesktop.org/show_bug.cgi?id=111009 with applied patch and
it doesn't crash and successfully passes.
Thanks!

Tested-by: Paul Chelombitko 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] intel/ir: Fix CFG corruption in opt_predicated_break().

[Francisco Jerez]

Specifically the optimization of a conditional BREAK + WHILE sequence
into a conditional WHILE seems pretty broken.  The list of successors
of "earlier_block" (where the conditional BREAK was found) is emptied
and then re-created with the same edges for no apparent reason.  On
top of that the list of predecessors of the block immediately after
the WHILE loop is emptied, but only one of the original edges will be
added back, which means that potentially several blocks that still
have it on their list of successors won't be on its list of
predecessors anymore, causing all sorts of hilarity due to the
inconsistency in the control flow graph.

The solution is to remove the code that's removing valid edges from
the CFG.  cfg_t::remove_block() will already clean up after itself.
The assert in bblock_t::combine_with() also needs to be removed since
we will be merging a block with multiple children into the first one
of them.

Found the issue on a hardware enabling branch originally, but
apparently somebody reproduced the same problem independently on
master in the meantime.

Fixes: d13bcdb3a9f ("i965/fs: Extend predicated break pass to predicate WHILE.")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111009
Cc: jiradet...@gmail.com
Cc: Sergii Romantsov 
Cc: Matt Turner 
Cc: mesa-sta...@lists.freedesktop.org
---
 src/intel/compiler/brw_cfg.cpp  | 3 ---
 src/intel/compiler/brw_predicated_break.cpp | 6 --
 2 files changed, 9 deletions(-)

diff --git a/src/intel/compiler/brw_cfg.cpp b/src/intel/compiler/brw_cfg.cpp
index 600b428a492..6c40889088d 100644
--- a/src/intel/compiler/brw_cfg.cpp
+++ b/src/intel/compiler/brw_cfg.cpp
@@ -128,9 +128,6 @@ void
 bblock_t::combine_with(bblock_t *that)
 {
assert(this->can_combine_with(that));
-   foreach_list_typed (bblock_link, link, link, >children) {
-  assert(link->block == that);
-   }
foreach_list_typed (bblock_link, link, link, >parents) {
   assert(link->block == this);
}
diff --git a/src/intel/compiler/brw_predicated_break.cpp 
b/src/intel/compiler/brw_predicated_break.cpp
index 607715dace4..e60052f3608 100644
--- a/src/intel/compiler/brw_predicated_break.cpp
+++ b/src/intel/compiler/brw_predicated_break.cpp
@@ -128,14 +128,8 @@ opt_predicated_break(backend_shader *s)
  while_inst->predicate = jump_inst->predicate;
  while_inst->predicate_inverse = !jump_inst->predicate_inverse;
 
- earlier_block->children.make_empty();
- earlier_block->add_successor(s->cfg->mem_ctx, while_block);
-
  assert(earlier_block->can_combine_with(while_block));
  earlier_block->combine_with(while_block);
-
- earlier_block->next()->parents.make_empty();
- earlier_block->add_successor(s->cfg->mem_ctx, earlier_block->next());
   }
 
   progress = true;
-- 
2.22.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev