[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 3:08 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:

> 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.
>

Matt, should I consider both of these r-b you?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140811/de0ab192/attachment.html>