From mboxrd@z Thu Jan 1 00:00:00 1970
From: bugzilla-daemon-CC+yJ3UmIYqDUpFQwHEjaQ@public.gmane.org
Subject: [Bug 90887] PhiMovesPass in register allocator broken
Date: Sat, 22 Aug 2015 20:15:06 +0000
Message-ID:
References:
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0372568154=="
Return-path:
In-Reply-To:
List-Unsubscribe: ,
List-Archive:
List-Post:
List-Help:
List-Subscribe: ,
Errors-To: nouveau-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Sender: "Nouveau"
To: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
List-Id: nouveau.vger.kernel.org
--===============0372568154==
Content-Type: multipart/alternative; boundary="1440274506.4f63d2E3.13329"; charset="UTF-8"
--1440274506.4f63d2E3.13329
Date: Sat, 22 Aug 2015 20:15:06 +0000
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
https://bugs.freedesktop.org/show_bug.cgi?id=90887
--- Comment #37 from Ilia Mirkin ---
(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.
--1440274506.4f63d2E3.13329
Date: Sat, 22 Aug 2015 20:15:06 +0000
MIME-Version: 1.0
Content-Type: text/html; charset="UTF-8"
Comment # 37
on bug 90887
from Ilia Mirkin
(In reply to jr from comment #36)
> Created attachment 117865 [details] [review] [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.
--1440274506.4f63d2E3.13329--
--===============0372568154==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KTm91dmVhdSBt
YWlsaW5nIGxpc3QKTm91dmVhdUBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZy
ZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL25vdXZlYXUK
--===============0372568154==--