[Mesa-dev] [PATCH] st/mesa: treat a write as a read for range purposes

[Ilia Mirkin]

On Sun, Jan 31, 2016 at 7:29 PM, Dave Airlie <airlied at gmail.com> wrote:
> On 30 January 2016 at 07:00, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> We use this logic to detect live ranges and then do plain renaming
>> across the whole codebase. As such, to prevent WaW hazards, we have to
>> treat a write as if it were also a read.
>>
>> For example, the following sequence was observed before this patch:
>>
>>  13: UIF TEMP[6].xxxx :0
>>  14:   ADD TEMP[6].x, CONST[6].xxxx, -IN[3].yyyy
>>  15:   RCP TEMP[7].x, TEMP[3].xxxx
>>  16:   MUL TEMP[3].x, TEMP[6].xxxx, TEMP[7].xxxx
>>  17:   ADD TEMP[6].x, CONST[7].xxxx, -IN[3].yyyy
>>  18:   RCP TEMP[7].x, TEMP[3].xxxx
>>  19:   MUL TEMP[4].x, TEMP[6].xxxx, TEMP[7].xxxx
>>
>> While after this patch it becomes:
>>
>>  13: UIF TEMP[7].xxxx :0
>>  14:   ADD TEMP[7].x, CONST[6].xxxx, -IN[3].yyyy
>>  15:   RCP TEMP[8].x, TEMP[3].xxxx
>>  16:   MUL TEMP[4].x, TEMP[7].xxxx, TEMP[8].xxxx
>>  17:   ADD TEMP[7].x, CONST[7].xxxx, -IN[3].yyyy
>>  18:   RCP TEMP[8].x, TEMP[3].xxxx
>>  19:   MUL TEMP[5].x, TEMP[7].xxxx, TEMP[8].xxxx
>>
>> Most importantly note that in the first example, the second RCP is done
>> on the result of the MUL while in the second, the second RCP should have
>> the same value as the first. Looking at the GLSL source, it is apparent
>> that both of the RCP's should have had the same source.
>>
>> Looking at what's going on, the GLSL looks something like
>>
>>   float tmin_8;
>>   float tmin_10;
>>   tmin_10 = tmin_8;
>> ... lots of code ...
>>   tmin_8 = tmpvar_17;
>> ... more code that never looks at tmin_8 ...
>>
>> And so we end up with a last_read somewhere at the beginning, and a
>> first_write somewhere at the bottom. For some reason DCE doesn't remove
>> it, but even if that were fixed, DCE doesn't handle 100% of cases, esp
>> including loops.
>>
>> With the last_read somewhere high up, we overwrite the previously
>> correct (and large) last_read with a low one, and then proceed to decide
>> to merge all kinds of junk onto this temp.
>>
>> As a result, we should treat a write as a last_read for the purpose of
>> determining the live range.
>>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: mesa-stable at lists.freedesktop.org
>
> I think you've convinced me this is right, I'd like to know why DCE
> doesn't trash
> that shader better, but I'm happy for this to go in.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>

Thanks! I'm going to leave this on the list until tomorrow in case
anyone else has feedback... these things are subtle and I don't want
to be doing the wrong thing. However I really do believe it's the
correct fix to deal with WaW hazards that otherwise appear.

  -ilia