[Mesa-dev] [PATCH 1/2] i965: Bail on vec4 copy propagation for scratch writes with source modifiers

[Anuj Phogat]

On Tue, Jul 29, 2014 at 7:18 AM, Matt Turner <mattst88 at gmail.com> wrote:
>
> On Mon, Jul 28, 2014 at 8:47 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> > Fixes Khronos GLES3 CTS test:
> > dynamic_expression_array_access_vertex
> >
> > Cc: <mesa-stable at lists.freedesktop.org>
> > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > ---
> > I don't have a test case though it might be useful to also include
> > the check for VS_OPCODE_URB_WRITE here?
> >
> >  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> > index 390448a..5a684a0 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> > @@ -273,6 +273,10 @@ try_copy_propagate(struct brw_context *brw, vec4_instruction *inst,
> >     if (has_source_modifiers && value.type != inst->src[arg].type)
> >        return false;
> >
> > +   if (has_source_modifiers &&
> > +       inst->opcode == SHADER_OPCODE_GEN4_SCRATCH_WRITE)
> > +      return false;
> > +
> >     bool is_3src_inst = (inst->opcode == BRW_OPCODE_LRP ||
> >                          inst->opcode == BRW_OPCODE_MAD ||
> >                          inst->opcode == BRW_OPCODE_BFE ||
> > --
>
> I think the thing to do here is to prevent scratch writes from having
> source modifiers. They're sends from MRFs, so adding it to
> is_send_from_grf seems wrong. I'd add it to the list in
> brw_shader.cpp:can_do_source_mods.
>
It does look like a right place to add the opcode. But, adding it to the
list in brw_shader.cpp:can_do_source_mods will bail on copy
propagation for few other cases which we don't want to exclude. See
following code in try_copy_propagate() in brw_vec4_copy_propagation.cpp:

   if ((has_source_modifiers || value.file == UNIFORM ||
        value.swizzle != BRW_SWIZZLE_XYZW) && !inst->can_do_source_mods(brw))
      return false;

Similar code also exists in fs_visitor::try_copy_propagate().

VS instruction count comparison for GLES3 CTS test:
With Mesa unchanged: 119
With my patch: 128
With suggested changes: 137


> VS_OPCODE_URB_WRITE shouldn't need any of this.