Comment # 25
on bug 90887
from jr
(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] [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