[Mesa-dev] [PATCH 03/29] i965: Define consistent interface to disable control flow execution masking.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Monday, May 11, 2015 05:12:41 PM Matt Turner wrote:
>> On Tue, May 5, 2015 at 9:57 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> > Kenneth Graunke <kenneth at whitecape.org> writes:
>> >
>> >> On Saturday, May 02, 2015 06:29:30 PM Francisco Jerez wrote:
>> >>> ---
>> >>>  src/mesa/drivers/dri/i965/brw_ir_fs.h   | 10 ++++++++++
>> >>>  src/mesa/drivers/dri/i965/brw_ir_vec4.h |  9 +++++++++
>> >>>  2 files changed, 19 insertions(+)
>> >>>
>> >>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> >>> index ce23fc5..6c65632 100644
>> >>> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> >>> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> >>> @@ -258,4 +258,14 @@ public:
>> >>>     bool pi_noperspective:1;   /**< Pixel interpolator noperspective flag */
>> >>>  };
>> >>>
>> >>> +/**
>> >>> + * Disable per-channel control flow execution masking on \p inst.
>> >>> + */
>> >>> +static inline fs_inst *
>> >>> +exec_all(fs_inst *inst)
>> >>> +{
>> >>> +   inst->force_writemask_all = true;
>> >>> +   return inst;
>> >>> +}
>> >>> +
>> >>>  #endif
>> >>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_vec4.h b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> >>> index 36a8224..48dd90f 100644
>> >>> --- a/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> >>> +++ b/src/mesa/drivers/dri/i965/brw_ir_vec4.h
>> >>> @@ -192,6 +192,15 @@ public:
>> >>>     }
>> >>>  };
>> >>>
>> >>> +/**
>> >>> + * Disable per-channel control flow execution masking on \p inst.
>> >>> + */
>> >>> +inline vec4_instruction *
>> >>> +exec_all(vec4_instruction *inst)
>> >>> +{
>> >>> +   inst->force_writemask_all = true;
>> >>> +   return inst;
>> >>> +}
>> >>>  } /* namespace brw */
>> >>>
>> >>>  #endif
>> >>>
>> >>
>> >> Patches 3-6 are:
>> >> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> >>
>> > Thanks Ken.
>> >
>> >> Matt and I were confused about the "exec_" prefix in the functions
>> >> though - why "exec"?  "exec_all" makes a lot of sense here - execute on
>> >> all channels - but "exec_saturate" seems a little odd to me.
>> >>
>> >> Perhaps we can find a different prefix (or you can convince us).
>> >
>> > *Shrug*, I felt that e.g. saturate() alone would have been misleading
>> > because the function doesn't saturate a value by itself, instead it
>> > modifies the execution parameters of an instruction.  I'm open to
>> > suggestions if you can come up with a nicer prefix.
>> 
>> I think set_* would be better.
>
> Agreed.  Reading this in action...
>
>    exec_all(bld.MOV(...))
>
> sounds a bit funny to me - execute this MOV?
>
> set_exec_all or set_writemask_all(MOV(...)) reads a bit nicer - set this
> property on the given MOV instruction...

Sounds good to me.  I've changed this in my image-load-store-nir branch
[1], do I need to resend?

[1] http://cgit.freedesktop.org/~currojerez/mesa/log/?h=image-load-store-nir
-------------- 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/20150512/ce357002/attachment.sig>