[Mesa-dev] [PATCH 2/3] i965: Lower min/max after optimization on Gen4/5.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Thu, Feb 11, 2016 at 4:41 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> Gen4/5's SEL instruction cannot use conditional modifiers, so min/max
>> are implemented as CMP + SEL. Handling that after optimization lets us
>> CSE more.
>>
>> On Ironlake:
>>
>>    total instructions in shared programs: 6426035 -> 6422753 (-0.05%)
>>    instructions in affected programs: 326604 -> 323322 (-1.00%)
>>    helped: 1411
>>
>>    total cycles in shared programs: 129184700 -> 129101586 (-0.06%)
>>    cycles in affected programs: 18950290 -> 18867176 (-0.44%)
>>    helped: 2419
>>    HURT: 328
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp           | 37 +++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_fs.h             |  1 +
>>  src/mesa/drivers/dri/i965/brw_fs_builder.h     | 10 ++-----
>>  src/mesa/drivers/dri/i965/brw_fs_nir.cpp       | 20 +++-----------
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp         | 38 ++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_vec4.h           |  2 ++
>>  src/mesa/drivers/dri/i965/brw_vec4_builder.h   | 10 ++-----
>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 14 ++--------
>>  8 files changed, 88 insertions(+), 44 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 0ce7ed1..e83f0ba 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -3475,6 +3475,36 @@ fs_visitor::lower_integer_multiplication()
>>     return progress;
>>  }
>>
>> +bool
>> +fs_visitor::lower_minmax()
>> +{
>> +   assert(devinfo->gen < 6);
>> +
>> +   bool progress = false;
>> +
>> +   foreach_block_and_inst_safe(block, fs_inst, inst, cfg) {
>> +      const fs_builder ibld(this, block, inst);
>> +
>> +      if (inst->opcode == BRW_OPCODE_SEL &&
>> +          inst->predicate == BRW_PREDICATE_NONE) {
>> +         assert(inst->conditional_mod == BRW_CONDITIONAL_GE ||
>> +                inst->conditional_mod == BRW_CONDITIONAL_L);
>
> Ken asked at the office if this assertion is necessary. I think it is.
> The PRM doesn't say anything about SEL with conditional modifiers
> other than .ge or .l.

I'm pretty sure it's not, the SEL instruction works fine with other
conditional mods, and I've found it moderately useful in the past.  And
at least the internal hardware docs mention explicitly that conditional
mods other than .l and .ge follow the cmp rules (rather than the cmpn
rules), which implies they're allowed...

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160213/1354355e/attachment-0001.sig>