[Mesa-dev] r600/sb loop issue

[Vadim Girlin]

On 12/12/2014 05:28 PM, Alex Deucher wrote:
> On Wed, Dec 10, 2014 at 6:50 AM, Vadim Girlin <vadimgirlin at gmail.com> wrote:
>> On 12/09/2014 07:39 AM, Vadim Girlin wrote:
>>>
>>> On 12/09/2014 05:18 AM, Dave Airlie wrote:
>>>>
>>>> On 8 December 2014 at 20:41, Vadim Girlin <vadimgirlin at gmail.com> wrote:
>>>>>
>>>>> 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.
>>>>>
>>>> This does seem to fix the problems in piglit, and looks close to what
>>>> I was attempting but written by someone who knows what they are doing :-)
>>>>
>>>> What is the sb_sched.cpp change for at the end for?
>>>
>>>
>>> It fixes those scheduler/regalloc errors for switch tests.
>>>
>>> Unfortunately, now I've installed some benchmarks for testing and AFAICS
>>> this patch breaks at least lightsmark 2008, so it seems the condition
>>> removed by the patch was there for a reason.
>>>
>>> I'll probably try to come up with better fix.
>>
>>
>> New patch is attached, the only difference is in the sb_sched.cpp (it
>> disables copy coalescing for some "unsafe" cases, so it may leave more MOVs
>> than previously, but I don't think there will be any noticeable effect on
>> performance).
>>
>> So far I don't see any problems with it, but I don't have many GL apps on
>> the test machine. At least lightsmark and unigine demos work for me.
>>
>
> Based on my limited understanding of the code:
>
> Acked-by: Alex Deucher <alexander.deucher at amd.com>

Alex, thanks for the review, I understand you wanted it to get into mesa 
release, but it really needs careful testing with more apps, so far I 
hoped Dave would do it as long as he's looking into these issues anyway. 
In theory I can also install steam on the test machine and some games, 
it just needs the time and I'm not sure if I'll find it, so far my main 
job is sufficient to make me pretty tired.

Current scheduler in SB is very fragile after adding handling for all 
special cases discovered during initial debugging etc, I said since the 
very beginning that I'd like to rewrite it, if only I had time. So any 
change like this can potentially break some apps even if piglit passes, 
and I'm not ready to take responsibility for that if I commit it myself, 
I just don't have time to deal with all possible consequences on all 
supported chips.

If you think it's ok, just push this patch (it requires revert of the 
previous Dave's commit 7b0067d2). I'm really sorry that I can't do more 
to help with it.

Vadim


>
>> Vadim
>>
>>
>>>
>>> Vadim
>>>
>>>>
>>>> Dave.
>>>>
>>>
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>