[Bug 90887] PhiMovesPass in register allocator broken - bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ

From: bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ@public.gmane.org
To: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: [Bug 90887] PhiMovesPass in register allocator broken
Date: Sat, 22 Aug 2015 20:15:06 +0000	[thread overview]
Message-ID: <bug-90887-8800-2SG7yfZb7q@http.bugs.freedesktop.org/> (raw)
In-Reply-To: <bug-90887-8800-V0hAGp6uBxMKqLRl/0Ahz6D7qz1kEfGD2LY78lusg7I@public.gmane.org/>

[-- Attachment #1.1: Type: text/plain, Size: 3055 bytes --]

https://bugs.freedesktop.org/show_bug.cgi?id=90887

--- Comment #37 from Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> ---
(In reply to jr from comment #36)
> Created attachment 117865 [details] [review]
> Edge type fix for prototype patch
> 
> With this patch on top of the prototype patch edge splitting during
> PhiMovesPass should no longer change non forward edges to forward. I think
> with current master this isn't a problem, because the split edge will always
> be a forward edge, but this still isn't a valid assumption.
> 
> After adding the following patch the fs-critical-edge test shows that edge
> splitting now preserves the edge type. This patch should also solve the 'all
> incoming edges are forward edges' problem that may be the cause of the
> missing blocks.
> 
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> index f153674..753bcbf 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> @@ -2838,7 +2838,7 @@ Converter::handleInstruction(const struct
> tgsi_full_instruction *insn)
>        }
> 
>        if (prevBB->getExit()->op == OP_BRA) {
> -         prevBB->cfg.attach(&convBB->cfg, Graph::Edge::FORWARD);
> +         prevBB->cfg.attach(&convBB->cfg, Graph::Edge::TREE);
>           prevBB->getExit()->asFlow()->target.bb = convBB;
>        }
>        setPosition(convBB, true);

Actually the edge types are much much more wrong, unfortunately. I don't know
how much it matters. For example in an if/else situation, i.e.

    *
   / \
  /   \
 *     *
  \   /
   \ /
    *

You should end up with 3 TREE edges and 1 CROSS edge. However nouveau happily
makes both of the "bottom" edges FORWARD. I guess you fix one of them to be
TREE, but the other is still going to be FORWARD instead of CROSS.

I think this is too big of a problem to fix in a fixup, esp since we don't
really know what these are used for.

I'm thinking the following:

(a) Push my fixup patch with the hash map, cc'd to stable. It's a little
inefficient, but it *works*, and is (in my mind) quite simple. I'll
double-check that it doesn't affect perf too much.
(b) Figure out this edge type insanity, including fixing things up, and maybe
adding a validator that makes sure that the edge types are correct.
(c) Revert my fixup, and implement actual edge splitting, (i.e. what you did)
and based on critical edges rather than the current logic.
(d) Lots and lots of testing.

Does that sound reasonable? One of the considerations here is that neither you
nor I have a *ton* of time to play around with this, and Mesa 11.0.0 will be
released in mid-September, and I'd like to have *some* fix for nv50 in there.

As for your patch, the edge is preserved in all cases except forward, which
becomes cross. (draw it out, should make sense, unless I messed up)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.

[-- Attachment #1.2: Type: text/html, Size: 4316 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau