On Tue, Jan 17, 2017 at 11:40 AM, Francisco Jerez <curroje...@riseup.net> wrote:
> Kenneth Graunke <kenn...@whitecape.org> writes:
>
>> (Co-authored by Matt Turner.)
>>
>> Image atomics, for example, return a value - but the shader may not
>> want to use it. We assigned a useless VGRF destination. This seemed
>> harmless, but it can actually be quite harmful. The register allocator
>> has to assign that VGRF to a real register. It may assign the same
>> actual GRF to the destination of an instruction that follows soon after.
>>
>> This results in a write-after-write (WAW) dependency, and stall.
>>
>> A number of "Deus Ex: Mankind Divided" shaders use image atomics, but
>> don't use the return value. Several of these were hitting WAW stalls
>> for nearly 14,000 (poorly estimated) cycles a pop. Making dead code
>> elimination null out the destination avoids this issue.
>>
>> This patch cuts one shader's estimated cycles by -98.39%! Removing the
>> message response should also help with data cluster bandwidth.
>>
>> On Skylake:
>>
>> total instructions in shared programs: 13366907 -> 13363051 (-0.03%)
>> instructions in affected programs: 49635 -> 45779 (-7.77%)
>> helped: 133
>> HURT: 0
>>
>> total cycles in shared programs: 255433388 -> 248081818 (-2.88%)
>> cycles in affected programs: 12370702 -> 5019132 (-59.43%)
>> helped: 100
>> HURT: 24
>>
>> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org>
>> ---
>> .../dri/i965/brw_fs_dead_code_eliminate.cpp | 56
>> ++++++++++++++++------
>> 1 file changed, 41 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> index 930dc733b45..885ae2638a8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> @@ -34,6 +34,43 @@
>> * yet in the tail end of this block.
>> */
>>
>> +static bool
>> +can_eliminate(const fs_inst *inst, BITSET_WORD *flag_live)
>> +{
>> + return inst->opcode != BRW_OPCODE_IF &&
>> + inst->opcode != BRW_OPCODE_WHILE &&
>
> Isn't this missing a bunch of other control flow instructions that
> shouldn't be eliminated? What's going on here, are they also being
> DCE'ed? Shouldn't this be !inst->is_control_flow()?
IF/WHILE were checked specifically because on Sandybridge they can
take a conditional-mod and a null destination.
Other control-flow instructions (or even IF & WHILE without cmod) have
BAD_FILE as a destination. Now that I think about it, I'm not really
sure why if.cmod wouldn't have had a BAD_FILE destination as well...
Since we never emit IF/WHILE with cmod anymore, it doesn't matter what
we do with the code. We should probably just make sure those
instructions have BAD_FILE dest. I'll be happy to clean this up after
these patches go in.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
