[Mesa-dev] [PATCH 02/22] i965/fs: add helper to retrieve instruction data size

[Juan A. Suarez Romero]

On Mon, 2017-01-09 at 15:47 -0800, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
> > From: "Juan A. Suarez Romero" <jasuarez at igalia.com>
> > 
> > The execution data size is the biggest type size of any instruction
> > operand.
> > 
> > We will use it to know if the instruction deals with DF, because in
> > Ivy
> > we need to duplicate the execution size and regioning parameters.
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs.cpp  | 19 ++++++++++++++-----
> >  src/mesa/drivers/dri/i965/brw_ir_fs.h |  1 +
> >  2 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > index c8a0693..eb3b4aa 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > @@ -340,6 +340,19 @@ fs_inst::has_source_and_destination_hazard()
> > const
> >     }
> >  }
> >  
> > +unsigned
> > +fs_inst::exec_data_size() const
> 
> Don't see any reason for this to be a method rather than an inline
> function.  Also the name seems kind of misleading -- Isn't this
> trying
> to calculate the size of the execution type of the instruction?  Why
> not
> call it exec_type_size() in that case?

Right. I'll make it an inline function, and change the name. I think I
chosen that name because it resembles to the Execution Data Type, and
hence the exec_data_size.

But the name you suggest seems more appropriate. Thanks!

> 
> > +{
> > +  unsigned exec_data_size = 0;
> > +
> > +  for (int i = 0; i < this->sources; i++) {
> > +    if (this->src[i].type != BAD_FILE)
> > +      exec_data_size = MAX2(exec_data_size, type_sz(this-
> > >src[i].type));
> > +  }
> > +
> > +  return exec_data_size;
> > +}
> > +
> >  bool
> >  fs_inst::is_copy_payload(const brw::simple_allocator &grf_alloc)
> > const
> >  {
> > @@ -4577,11 +4590,7 @@ get_fpu_lowered_simd_width(const struct
> > gen_device_info *devinfo,
> >         !inst->force_writemask_all) {
> >        const unsigned channels_per_grf = inst->exec_size /
> >           DIV_ROUND_UP(inst->size_written, REG_SIZE);
> > -      unsigned exec_type_size = 0;
> > -      for (int i = 0; i < inst->sources; i++) {
> > -         if (inst->src[i].file != BAD_FILE)
> > -            exec_type_size = MAX2(exec_type_size, type_sz(inst-
> > >src[i].type));
> > -      }
> > +      unsigned exec_type_size = inst->exec_data_size();
> 
> You can make this declaration const while you're at it.

OK. 

> 
> >        assert(exec_type_size);
> >  
> >        /* The hardware shifts exactly 8 channels per compressed
> > half of the
> > diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > index cad3712..9875f2d 100644
> > --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> > @@ -349,6 +349,7 @@ public:
> >     bool can_change_types() const;
> >     bool has_side_effects() const;
> >     bool has_source_and_destination_hazard() const;
> > +   unsigned exec_data_size() const;
> >  
> >     /**
> >      * Return the subset of flag registers read by the instruction
> > as a bitset
> > -- 
> > 2.9.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev