[Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations

[Davin McCall]

On 29/06/15 13:26, Francisco Jerez wrote:
> Davin McCall <davmac at davmac.org> writes:
>
>> On 29/06/15 10:40, Francisco Jerez wrote:
>>> Davin McCall <davmac at davmac.org> writes:
>>>
>>>> On 26/06/15 14:53, Francisco Jerez wrote:
>>>>
>>>>> [...]
>>>>>
>>>>> Your first approach seemed quite reasonable IMHO.  Were you able to
>>>>> measure any performance regression from it?
>>>>>
>>>>> Thanks.
>>>>>
>>>> When I run an apitrace replay of a Dota 2 trace [1] with
>>>> LIBGL_ALWAYS_SOFTWARE and without the patch I get (averaged over 5 runs):
>>>>
>>>>        Maximum Resident Set Size (kbytes): 4509696
>>>>        FPS: .9044752
>>>>        user time: 2467.306
>>>>
>>>> ("Maximum Resident Set Size" and user time are given by GNU "time". I'm
>>>> not sure what it's really measuring, because this is a 32-bit system and
>>>> I don't see how the maximum resident set could be > 4GB; "top" shows
>>>> virt+res capping out at about 2.3GB. However I assume MRSS is at least
>>>> giving some relative indication of memory use; the deviation wasn't too
>>>> high).
>>>>
>>>> With the patch (again averaged over 5 runs):
>>>>
>>>>        Maximum Resident Set Size: 4523622.4
>>>>        FPS: 0.9068524
>>>>        user time: 2457.506
>>>>
>>>> So, "MRSS" has gone up a bit, but nothing else has changed
>>>> significantly. I think that means memory use has slightly increased, but
>>>> performance hasn't really changed.
>>>>
>>>> I wanted to test with the Intel driver using INTEL_NO_HW, but I get a
>>>> segfault when the patch is applied. Having checked over the patch
>>>> several times, I think this might mean that it triggers a latent bug
>>>> elsewhere, but I am still investigating that. V2 of the patch does not
>>>> trigger this crash.
>>>>
>>> Most likely some assumption left to fix in the i965 back-end?
>> That's what I thought. However, I've just tried (after reverting the
>> patch) inserting a single field in the exec_list structure to emulate
>> the data layout that should occur when the patch is applied - but I
>> can't then reproduce the problem. So I guess there is something wrong
>> with the patch itself, but I'm blind to it :(
>>
>>>     Have you
>>> shared your changes to the i965 driver already?  They don't seem to be
>>> part of your v1.
>> No, I've not shared them previously, but I've included them now (below).
>>
>> Davin
>>
>> [...]
>>
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> b/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> index 58ac598..eeabd08 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_builder.h
>> @@ -85,7 +85,8 @@ namespace brw {
>>          fs_builder
>>          at_end() const
>>          {
>> -         return at(NULL, (exec_node *)&shader->instructions.tail);
>> +         return at(NULL,
>> +               (exec_node *)&shader->instructions.tail_sentinel.prev);
> This looks wrong, the previous code was taking a pointer to the tail
> sentinel, but (exec_node *)&tail_sentinel.prev != &tail_sentinel.
>

You're right, thanks. However, when I fix this an assert triggers:

    glmark2: brw_fs_generator.cpp:2136: int
    fs_generator::generate_code(const cfg_t*, int): Assertion `!"Should
    be lowered by lower_load_payload()"' failed.

I'll have to keep investigating.

Davin

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150629/9aed54ea/attachment.html>