[Mesa-dev] [PATCH 10/19] i965/fs: Lower LOAD_PAYLOAD and clean up.

[Kenneth Graunke]

On Tuesday, May 27, 2014 06:47:41 PM Matt Turner wrote:
> Clean up with with register_coalesce()/dead_code_eliminate().
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 42 
++++++++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 +
>  2 files changed, 43 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 0856b6b..c0af6d0 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2574,6 +2574,43 @@ fs_visitor::lower_uniform_pull_constant_loads()
>     }
>  }
>  
> +bool
> +fs_visitor::lower_load_payload()
> +{
> +   bool progress = false;
> +
> +   foreach_list_safe(node, &instructions) {
> +      fs_inst *inst = (fs_inst *)node;
> +
> +      if (inst->opcode == SHADER_OPCODE_LOAD_PAYLOAD) {
> +         fs_reg dst = inst->dst;
> +
> +         /* The generator creates the message header if present, which is 
in
> +          * the first register of the message payload.
> +          */
> +         if (!inst->header_present) {

I don't think this will work like you want.  In your emit_texture_gen7 patch, 
when the texturing operation needs a header, you set src[0] to reg_undef 
(BAD_FILE).  But you don't set header_present on the LOAD_PAYLOAD instruction.

Here, inst is the LOAD_PAYLOAD operation...so you're checking header_present
on...not what you thought.  It sure looks like this will generate

MOV(dst, reg_undef)

which is bad.

I think you actually should just go with your original version of this patch:
http://article.gmane.org/gmane.comp.video.mesa3d.devel/76592

AFAICT, the semantics of LOAD_PAYLOAD are:

/**
 * Combines multiple sources of size 1 into a larger virtual GRF.
 * For example, parameters for a send-from-GRF message.  Or, updating
 * channels of a size 4 VGRF used to store vec4s such as texturing results.
 *
 * This will be lowered into MOVs from each source to consecutive reg_offsets
 * of the destination VGRF.
 *
 * src[0] may be BAD_FILE.  If so, the lowering pass skips emitting the MOV,
 * but still reserves the first channel of the destination VGRF.  This can be
 * used to reserve space for, say, a message header set up by the generators.
 */

I'd like a comment like that to appear above the SHADER_OPCODE_LOAD_PAYLOAD 
opcode in brw_defines.h.  Feel free to improve the wording.

> +            inst->insert_before(MOV(dst, inst->src[0]));
> +         } else {
> +            assert(inst->src[0].file == BAD_FILE);
> +         }
> +         dst.reg_offset++;
> +
> +         for (int i = 1; i < inst->sources; i++) {
> +            inst->insert_before(MOV(dst, inst->src[i]));
> +            dst.reg_offset++;
> +         }
> +
> +         inst->remove();
> +         progress = true;
> +      }
> +   }
> +
> +   if (progress)
> +      invalidate_live_intervals();
> +
> +   return progress;
> +}
> +
>  void
>  fs_visitor::dump_instructions()
>  {
> @@ -3071,6 +3108,11 @@ fs_visitor::run()
>           progress = OPT(compute_to_mrf) || progress;
>        } while (progress);
>  
> +      if (lower_load_payload()) {
> +         register_coalesce();
> +         dead_code_eliminate();
> +      }
> +
>        lower_uniform_pull_constant_loads();
>  
>        assign_curb_setup();
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h 
b/src/mesa/drivers/dri/i965/brw_fs.h
> index d0e459c..2b60945 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -391,6 +391,7 @@ public:
>     void fail(const char *msg, ...);
>     void no16(const char *msg, ...);
>     void lower_uniform_pull_constant_loads();
> +   bool lower_load_payload();
>  
>     void push_force_uncompressed();
>     void pop_force_uncompressed();
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140531/7ef2517d/attachment.sig>