[Mesa-dev] [PATCH] glsl: Detect do-while-false loops and unroll them

[Tapani Pälli]

On 02/23/2016 03:23 AM, Ian Romanick wrote:
> On 02/22/2016 04:16 PM, Ian Romanick wrote:
>> On 02/22/2016 04:05 PM, Matt Turner wrote:
>>> On Mon, Feb 22, 2016 at 11:42 AM, Ian Romanick <idr at freedesktop.org> wrote:
>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>
>>>> Previously loops like
>>>>
>>>>     do {
>>>>        // ...
>>>>     } while (false);
>>>>
>>>> that did not have any other loop-branch instructions would not be
>>>> unrolled.  This is commonly used to wrap multiline preprocessor macros.
>>>>
>>>> This produces IR like
>>>>
>>>>      (loop (
>>>>         ...
>>>>         break
>>>>      ))
>>>>
>>>> Since limiting_terminator was NULL, the loop unroller would
>>>> throw up its hands and say, "I don't know how many iterations.  How
>>>> can I unroll this?"
>>>>
>>>> We can detect this another way.  If there is no limiting_terminator
>>>> and the only loop-branch is a break as the last IR, there's only one
>>>> iteration.
>>>>
>>>> On my very old checkout of shader-db, this removes a loop from Orbital
>>>> Explorer, but it does not otherwise affect the shader.  The loop removed
>>>> is the one the compiler inserts surrounding the switch statement.
>>>
>>> Orbital Explorer has a dead while loop because of
>>>
>>> commit 73dd50acf6d244979c2a657906aa56d3ac60d550
>>> Author: Tapani Pälli <tapani.palli at intel.com>
>>> Date:   Wed Aug 6 09:46:54 2014 +0300
>>>
>>>      glsl: implement switch flow control using a loop
>>>
>>> I don't understand how this patch interacts with that nor why it
>>> doesn't break Orbital Explorer rendering (I checked).
>>>
>>> The Orbital Explorer shader *does* have other loop-branch
>>> instructions, so it seems like this patch shouldn't have affected it?
>>
>> I will dig into this more.  There are a lot of break instructions in the
>> "before" IR, and they all disappear.  There's something fishy going on...
>
> I see what's happening.  Each case in the switch-statement ends with a
> return instead of a break.  Eventually the function gets inlined and the
> returns are replaced with breaks.  However, this code causes the loop to
> be removed, so the returns aren't replaced with breaks.
>
> I spent probably an hour poking at this, and think the only way to hit
> it is to have a switch-statement in main that uses returns from the
> cases.  For all other cases, the returns get lowered before loop
> analysis occurs.
>
> I tried to come up with a test case involving conditional and
> unconditional returns from a switch with and without falling through...
> I couldn't come up with one that this patch broke.  Perhaps someone more
> clever can.  It really seems like the way this changes the IR should
> cause something to malfunction. :(

If I understand the problem correctly then this would possibly also mean 
that one is able to create a loop that will not work correctly? The main 
motivation to use loop to fix these cases was that loop flow control was 
working and handled correctly so we would not have to add switch to ir 
and possibly duplicate some logic what is done with loops already.


> Opinions about the patch?
>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>