[Mesa-dev] [PATCH] i965/fs: Don't propagate conditional modifiers from integer compares to adds

[Ian Romanick]

On 09/14/2018 02:52 AM, Alejandro Piñeiro wrote:
> No shader-db changes, so perhaps adding a test on
> test_fs_cmod_propagation? In any case, the patch looks good to me:

I should have mentioned in the commit message, but I added a piglit test:

https://patchwork.freedesktop.org/patch/249182/

> Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>
> 
> 
> On 14/09/18 00:06, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> No shader-db changes on any Intel platform... which probably explains
>> why no bugs have been bisected to this problem since it landed in Mesa
>> 18.1. :( The commit mentioned below is in 18.2, so 18.1 would need a
>> slightly different fix (due to code refactoring).
>>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> Fixes: 77f269bb560 "i965/fs: Refactor propagation of conditional modifiers from compares to adds"
>> Cc: Matt Turner <mattst88 at gmail.com> (reviewed the original patch)
>> Cc: Alejandro Piñeiro <apinheiro at igalia.com> (reviewed the original patch)
>> ---
>>  src/intel/compiler/brw_fs_cmod_propagation.cpp | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp
>> index 5b74f267359..5fb522f810f 100644
>> --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp
>> +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp
>> @@ -211,9 +211,17 @@ opt_cmod_propagation_local(const gen_device_info *devinfo, bblock_t *block)
>>        /* A CMP with a second source of zero can match with anything.  A CMP
>>         * with a second source that is not zero can only match with an ADD
>>         * instruction.
>> +       *
>> +       * Only apply this optimization to float-point sources.  It can fail for
>> +       * integers.  For inputs a = 0x80000000, b = 4, int(0x80000000) < 4, but
>> +       * int(0x80000000) - 4 overflows and results in 0x7ffffffc.  that's not
>> +       * less than zero, so the flags get set differently than for (a < b).
>>         */
>>        if (inst->opcode == BRW_OPCODE_CMP && !inst->src[1].is_zero()) {
>> -         progress = cmod_propagate_cmp_to_add(devinfo, block, inst) || progress;
>> +         if (brw_reg_type_is_floating_point(inst->src[0].type) &&
>> +             cmod_propagate_cmp_to_add(devinfo, block, inst))
>> +            progress = true;
>> +
>>           continue;
>>        }
>>