[Mesa-dev] [PATCH 1/2] i965/fs: Handle negating immediates on MADs when propagating saturates

[Ian Romanick]

On 11/20/2017 03:32 PM, Matt Turner wrote:
> On Mon, Nov 20, 2017 at 2:50 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 11/20/2017 02:33 PM, Matt Turner wrote:
>>> MADs don't take immediate sources, but we allow them in the IR since it
>>> simplifies a lot of things. I neglected to consider that case.
>>>
>>> Fixes: 4009a9ead490 ("i965/fs: Allow saturate propagation to propagate
>>>                       negations into MADs.")
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103616
>>> Reported-and-Tested-by: Ruslan Kabatsayev <b7.10110111 at gmail.com>
>>> ---
>>>  src/intel/compiler/brw_fs_saturate_propagation.cpp | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/intel/compiler/brw_fs_saturate_propagation.cpp b/src/intel/compiler/brw_fs_saturate_propagation.cpp
>>> index 1c97a507d8..d6cfa79a61 100644
>>> --- a/src/intel/compiler/brw_fs_saturate_propagation.cpp
>>> +++ b/src/intel/compiler/brw_fs_saturate_propagation.cpp
>>> @@ -88,8 +88,14 @@ opt_saturate_propagation_local(fs_visitor *v, bblock_t *block)
>>>                          scan_inst->src[0].negate = !scan_inst->src[0].negate;
>>>                          inst->src[0].negate = false;
>>>                       } else if (scan_inst->opcode == BRW_OPCODE_MAD) {
>>> -                        scan_inst->src[0].negate = !scan_inst->src[0].negate;
>>> -                        scan_inst->src[1].negate = !scan_inst->src[1].negate;
>>> +                        for (int i = 0; i < 2; i++) {
>>> +                           if (scan_inst->src[i].file == IMM) {
>>> +                              brw_negate_immediate(scan_inst->src[i].type,
>>> +                                                   &scan_inst->src[i].as_brw_reg());
>>> +                           } else {
>>> +                              scan_inst->src[i].negate = !scan_inst->src[i].negate;
>>> +                           }
>>> +                        }
>>
>> Is this going to affect the number of generated instructions if there
>> are multiple MADs using the same immediate value for a multiply source?
>> Would it be better to find a multiply source that isn't an immediate?
> 
> Interesting question. I think the answer is no, since
> brw_fs_combine_constants.cpp builds its list of constants by ignoring
> their signs. Since source negation is free, it just loads positive
> values into registers and negates them with a source modifier if
> needed.
> 
> I ran shader-db to confirm -- no changes on SKL.

In that case, this patch is also

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>