[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 14:16:33 +0000	[thread overview]
Message-ID: <bug-90887-8800-LCMt9SU15x@http.bugs.freedesktop.org/> (raw)
In-Reply-To: <bug-90887-8800-V0hAGp6uBxMKqLRl/0Ahz6D7qz1kEfGD2LY78lusg7I@public.gmane.org/>

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

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

--- Comment #35 from jr <j-r-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org> ---
(In reply to jr from comment #34)
> (In reply to Ilia Mirkin from comment #33)
> > (In reply to jr from comment #32)
> > > (In reply to Ilia Mirkin from comment #31)
> > > > Really if I could understand wtf the needNewElseBlock logic was trying to
> > > > do, and could construct a test shader to hit this in *regular* scenarios,
> > > > not just the lowered output of TXL, that would make me a lot more
> > > > comfortable with any approach that we pick.
> > > 
> > > Looking at the TGSI->IR translation I'd guess that needNewElseBlock is
> > > trying to detect the edge from the 'bare' unconditional jump in an if
> > > without else, seemingly because adding the new moves is not allowed (though
> > > I'm not sure why). At least it seems to be the only construct creating a
> > > graph satisfying the condition, AFAICT.
> > 
> > Well, in general you can't add work if you have a critical edge -- if an
> > block has multiple outgoing edges, then you can't put the work into that
> > block. If a block has multiple incoming edges, then you can't leave the work
> > in that block, so you have to create a separate block to put work that you
> > want done on that specific block transition.
> > 
> > However here it's looking for something much more specific.
> 
> Ok, that makes sense. It might be that the original author wasn't thinking
> about the problem in this terms and just wanted to fix the single
> problematic case at hand, i.e. if without else. Then splitting any critical
> edge would be the right solution, though in practice I don't think it'd
> trigger more often.

After looking at it some more I'm convinced that this is what the original
intent was. It also explains why the tree types of the new edges were
hardcoded. Because the (critical) edge being split was always a forward edge.

I may also have found your problem with the missing base blocks. If you look at
the log of the critical-edge test (it's easier to see in my version because it
has dumping of incoming edges enabled) you'll see that BB 4 and 6 have only
forward incoming edges and have been pruned (together with following blocks).
This is not directly related to the PhiMovesProblem, but is a result of the
TGSI->IR translation. One of the two edges inserted by TGSI_OPCODE_ENDIF should
be TREE. I don't know which one to choose. If we choose the condBB one than the
assumption of the PhiMovesPass that the split esge is always type FORWARD will
be wrong and the resulting graph will be wrong. I'll modify my prototype patch
in a second to take this into account correctly (and remove the type
hardcoding, but differently than I thought, it is the second edge that has to
keep the type), so that we can choose either way.

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

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

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