On 04/23/2014 11:39 PM, Matt Turner wrote:
> On Wed, Apr 23, 2014 at 11:25 PM, Kenneth Graunke <kenn...@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...
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
