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

[Matt Turner]

On Sat, Feb 13, 2016 at 2:02 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> 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...

Okay, right. The PRM says "and all other conditional modifiers follow
the cmp rules."

Which ones are be useful? .z/.nz/.o/.u don't make sense.

I see that the SEL documentation says

"""
For a sel instruction with a .l or .ge conditional modifier, if one
source is NaN and the other not NaN, the non-NaN source is the result.
If both sources are NaNs, the result is NaN. For all other conditional
modifiers, if either source is NaN then src1 is selected.
"""

So .ge/.l return non-NaN if one source is NaN, while .g/.le propagate NaNs.

We have mistakenly used the wrong conditional modifier before (see
commit 3b7f683f3).

If there's no code that wants the propagate-NaN behavior now, I'd
rather we remove the assertion when we expect to use that behavior.
Otherwise, it may just allow us to make a mistake.