[Mesa-dev] r600/sb loop issue

[Vadim Girlin]

On 12/06/2014 07:13 AM, Vadim Girlin wrote:
> On 12/04/2014 01:43 AM, Dave Airlie wrote:
>> Hi Vadim,
>>
>> I've been looking with Glenn's help into a bug in sb for a couple of
>> weeks now triggered by a change in how GLSL generates switch
>> statements.
>>
>> I understand you probably aren't too interested in r600g but I believe
>> I'm hitting a design level problem and I would like some advice.
>>
>> So it appears that GLSL can create loops that don't repeat for switch
>> statements, and it appears SB wasn't ready to handle such a thing.
>
> Hi, Dave,
>
> I suspect we should rather get rid of such loops somehow, i.e. convert
> to something else, the loop that never repeats is not really a loop
> anyway. AFAICS "continue" is not supported in switch statements
> according to GLSL specs, so the loops generated for switch will never be
> repeated. Am I missing something? Even if repeating is possible somehow,
> at least we can get rid of the loops that are not repeated.
>
> I think loops are less efficient than other control flow instructions on
> r600g hw (at least because they increase stack usage), and possibly on
> other hw too.
>
> In fact it seems sb basically gets rid of it already in IR, it just
> doesn't know how to translate resulting control flow to ISA, because so
> far it only supports specific control flow structure for if-then-else
> that was previously preserved during optimizations. I think it may be
> not very hard to implement support for that in finalizer, I'll look into
> it.

In fact handling that control flow in finalizer is not as easy as I 
hoped, probably impossible, at least if we want to make it efficient. I 
forgot about the limitations of R600 ISA.

OTOH it seems I've managed to fix the issues with loops, the patch is 
attached (it's meant to be used instead of 7b0067d2). There are no 
piglit regressions on evergreen, but I didn't test any real apps.

Vadim


>
>>
>> sb has the ->is_loop() and it just checks !repeats.empty(), so this
>> meant in the finalizer code we'd fall into the if statement which
>> would then assert.
>>
>> I hacked/fixed (more hacked), this in
>> 7b0067d23a6f64cf83c42e7f11b2cd4100c569fe
>> which attempts to detect single pass loops and handle things that way.
>>
>> However this lead to stack depth calculations being incorrectly done,
>> so I moved the single loop detect into the is_loop check, (see
>> attached patch).
>>
>> This fixes the rendering in some places, but lead to a regression in
>> tests/shaders/glsl-vs-continue-in-switch-in-do-while.shader_test
>> error at : PHI     t76||FP at R3.x,       t128||FP at R3.x, t115||FP at R3.x,
>> t102||FP at R3.x, t89||FP at R3.x : expected
>>                  operand value t115||FP at R3.x, gpr contains t17||FP at R3.x
>> error at : PHI     t76||FP at R3.x,       t128||FP at R3.x, t115||FP at R3.x,
>> t102||FP at R3.x, t89||FP at R3.x : expected
>>                  operand value t102||FP at R3.x, gpr contains t17||FP at R3.x
>>
>> Now Glenn suspected this was due to the is_loop check in
>> sb_shader.cpp:create_bbs,
>> and changing that check to only detect repeating loops removes that
>> issue,
>> but introduces stack sizing issues again, resulting in lockups/random
>> rendering.
>>
>> So I just want to ask had you considered single loops with an always
>> break in sb design,
>
> I didn't see such loops with any test cases, so I didn't even think
> about it.
>
>> and perhaps some idea where things are going so wrong with the
>> register alloc above.
>
> Not sure, but as long as the only "repeat" node is optimized away in
> bc_parser because it's useless due to unconditional break, I suspect it
> may be not easy to make all other code think that it's still a loop.
>
> I've tried a quick fix to not optimize the repeat away for such loops,
> but it results in other issues, probably it will require handling this
> as a special case in other places, so it doesn't look like a good idea
> either.
>
> I'll try to implement the solution that I described above, that is,
> translate resulting control flow back to ISA. If it won't be too much
> work, it's probably the best way and it won't use loop instructions in
> the end.
>
>>
>> I suspect I'll keep digging into this, but its getting to the edges of
>> the brain space/time I can find!
>>
>> Dave.
>>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-r600g-sb-fix-issues-with-loops-created-for-switch-st.patch
Type: text/x-patch
Size: 3971 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141208/cbc88ae8/attachment-0001.bin>