[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: Thu, 20 Aug 2015 22:16:24 +0000	[thread overview]
Message-ID: <bug-90887-8800-ZFgUiw5sQm@http.bugs.freedesktop.org/> (raw)
In-Reply-To: <bug-90887-8800-V0hAGp6uBxMKqLRl/0Ahz6D7qz1kEfGD2LY78lusg7I@public.gmane.org/>


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

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

--- Comment #25 from jr <j-r-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> ---
(In reply to Ilia Mirkin from comment #24)
> (In reply to jr from comment #23)
> > (In reply to Ilia Mirkin from comment #22)
> > > (In reply to jr from comment #21)
> > > > Created attachment 117077 [details] [review] [review] [review] [review]
> > > > Prototype of a more highlevel graph modification api
> > > > 
> > > > ...
> > > 
> > > Hrm, I was about to (try to) push this out, but it doesn't seem to work. I
> > > made a few local adjustments, like
> > > 
> > > RegAlloc::PhiMovesPass::isCriticalEdge(BasicBlock *b, BasicBlock *p)
> > > {
> > >    return b->cfg.incidentCount() > 1 && p->cfg.outgoingCount() > 1;
> > > }
> > > 
> > > ...
> > > 
> > > 
> > > where did BB:4 go? poof. not great :( this is with piglit's
> > > tests/shaders/ssa/fs-critical-edge.shader_test
> > > 
> > > I guess I'll go with either your or my first patches. Want to get this fixed
> > > for Mesa 11, which is going to get branched off some time tomorrow.
> > 
> > Sorry, most probably my prototype code is crap. I'll have an hour now to
> > look at it. I'll let you know if I find something. Otherwise I'm fine with
> > your first patch.
> 
> Awesome! While I don't know whether the isCriticalEdge thing is the right
> thing to do instead of the current needNewElseBlock logic, I do think that
> splitEdge should be able to work properly even if needNewElseBlock always
> returns true.

Yes, it should. My usual test cases plus the patch below look fine. My piglit
(from git://anongit.freedesktop.org/piglit) doesn't have a fs-critical-edge
test, though. Where do I find it?

That prototype patch was more of a RFC anyway to show how graph manipulation
could be made less intimidating. If you like the direction I will work some
more on it. But I think it is safe to go with your patch for now.


--- a/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp
+++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_ra.cpp
@@ -335,15 +335,7 @@ RegAlloc::BuildIntervalsPass::addLiveRange(Value *val,
 bool
 RegAlloc::PhiMovesPass::needNewElseBlock(BasicBlock *b, BasicBlock *p)
 {
-   if (b->cfg.incidentCount() <= 1)
-      return false;
-
-   int n = 0;
-   for (Graph::EdgeIterator ei = p->cfg.outgoing(); !ei.end(); ei.next())
-      if (ei.getType() == Graph::Edge::TREE ||
-          ei.getType() == Graph::Edge::FORWARD)
-         ++n;
-   return (n == 2);
+   return (b->cfg.incidentCount() > 1) && (p->cfg.outgoingCount() > 1);
 }

 // For each operand of each PHI in b, generate a new value by inserting a MOV

-- 
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: 4311 bytes --]

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

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