[Mesa-dev] [PATCH 13/19] i965/fs: Only consider real sources when comparing instructions.

[Kenneth Graunke]

On 04/23/2014 11:39 PM, Matt Turner wrote:
> On Wed, Apr 23, 2014 at 11:25 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> On 04/18/2014 11:56 AM, Matt Turner wrote:
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> I originally thought this needed to go earlier in the patch series,
>> since by this point you're emitting opcodes with more than 3 sources.
>> However, CSE ignores SHADER_OPCODE_LOAD_PAYLOAD, so this code will never
>> run for opcodes that could break.  So, it's probably fine.
>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> index 94f657d..e40567f 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>>> @@ -94,10 +94,20 @@ is_expression_commutative(enum opcode op)
>>>  }
>>>
>>>  static bool
>>> -operands_match(enum opcode op, fs_reg *xs, fs_reg *ys)
>>> +operands_match(fs_inst *a, fs_inst *b)
>>>  {
>>> -   if (!is_expression_commutative(op)) {
>>> -      return xs[0].equals(ys[0]) && xs[1].equals(ys[1]) && xs[2].equals(ys[2]);
>>> +   fs_reg *xs = a->src;
>>> +   fs_reg *ys = b->src;
>>> +
>>> +   if (!is_expression_commutative(a->opcode)) {
>>> +      bool match = true;
>>> +      for (int i = 0; i < a->sources; i++) {
>>> +         if (!xs[i].equals(ys[i])) {
>>> +            match = false;
>>> +            break;
>>> +         }
>>> +      }
>>> +      return match;
>>>     } else {
>>>        return (xs[0].equals(ys[0]) && xs[1].equals(ys[1])) ||
>>>               (xs[1].equals(ys[0]) && xs[0].equals(ys[1]));
>>
>> It strikes me as a bit asymmetric to have the first block handle an
>> arbitrary number of sources, and the second only check 0 and 1.  It
>> makes sense, since our commutative opcodes are all binops, but...
>>
>> How about adding
>>
>>       /* All commutative opcodes are binary operations. */
>>       assert(a->sources == 2 && b->sources == 2);
>>
>> here in the commutative case?
> 
> What is an example of a commutative non-binary operator?

I don't think we have any in i965, but things like min3/max3/mid3 are
three-source operations that are "commutative".

At any rate, adding the assertion makes it obvious that this code is
correct by design, and we didn't just forget to update it for
arbitrary-length source lists...I don't think it's an unreasonable
request...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140423/a8ef2189/attachment.sig>