[Mesa-dev] [PATCH 15/21] i965: Define consistent interface to predicate an instruction.

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Tue, Apr 28, 2015 at 10:08 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_ir_fs.h    | 22 ++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_ir_svec4.h | 26 ++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_ir_vec4.h  | 22 ++++++++++++++++++++++
>>  3 files changed, 70 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> index 1bbe164..b2dfa00 100644
>> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> @@ -329,4 +329,26 @@ exec_all(fs_inst *inst)
>>     return inst;
>>  }
>>
>> +/**
>> + * Make the execution of \p inst dependent on the evaluation of a possibly
>> + * inverted predicate.
>> + */
>> +static inline fs_inst *
>> +exec_predicate_inv(enum brw_predicate pred, bool inverse,
>> +                   fs_inst *inst)
>
> Is the order of the parameters for a particular reason? If not, I'd
> much rather this be
>
> exec_predicate_inv(fs_inst *inst, enum brw_predicate pred, bool inverse = false)
>
> since it feels more natural for inst to be first, and
> predicate-inverse is almost never true unless you start doing that a
> lot in later patches.

Hi Matt,

There is a reason indeed, it's not extremely important though so I'd be
okay with the argument order you propose.  The reason is that the "inst"
argument is typically a complex (often multi-line) expression so having
the "control" arguments right next to the function being applied seemed
to improve readability of the code considerably.  E.g.:

exec_predicate_inv(BRW_PREDICATE_NORMAL, true,
                   bld.OP(...,
                          ...,
                          ...));

Seemed more readable than:

exec_predicate_inv(bld.OP(...,
                          ...,
                          ...),
                   BRW_PREDICATE_NORMAL, true /* Where were these going
                                                 to again? */);

For the most common case where the predicate is not inverted there is a
separate convenience function exec_predicate() which just passes zero to
the "inverse" argument.

Thanks for your review!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150430/016c4ace/attachment.sig>