[Mesa-dev] [PATCH 4/9] i965: remove unused members in blorp clear program

[Pohjolainen, Topi]

On Thu, Nov 28, 2013 at 11:50:24PM -0800, Kenneth Graunke wrote:
> On 11/28/2013 11:44 PM, Kenneth Graunke wrote:
> > On 11/27/2013 01:13 PM, Topi Pohjolainen wrote:
> >> Documentation for R0 and R1 is taken from
> >> fs_visitor::setup_payload_gen6().
> >>
> >> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_blorp_clear.cpp | 15 +++------------
> >>  1 file changed, 3 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> >> index 2fa0b50..a937edb 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> >> +++ b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
> >> @@ -104,12 +104,6 @@ private:
> >>     const brw_blorp_const_color_prog_key *key;
> >>     struct brw_compile func;
> >>  
> >> -   /* Thread dispatch header */
> >> -   struct brw_reg R0;
> >> -
> >> -   /* Pixel X/Y coordinates (always in R1). */
> >> -   struct brw_reg R1;
> >> -
> > 
> > I like the fact that you're getting rid of these.  They're trivially
> > reconstructable as brw_vec8_grf(0, 0) and brw_vec8_grf(1, 0), so storing
> > them isn't really worthwhile.
> > 
> >>     /* Register with push constants (a single vec4) */
> >>     struct brw_reg clear_rgba;
> >>  
> >> @@ -123,8 +117,6 @@ brw_blorp_const_color_program::brw_blorp_const_color_program(
> >>     : mem_ctx(ralloc_context(NULL)),
> >>       brw(brw),
> >>       key(key),
> >> -     R0(),
> >> -     R1(),
> >>       clear_rgba(),
> >>       base_mrf(0)
> >>  {
> >> @@ -363,11 +355,8 @@ brw_blorp_const_color_params::get_wm_prog(struct brw_context *brw,
> >>  void
> >>  brw_blorp_const_color_program::alloc_regs()
> >>  {
> >> -   int reg = 0;
> >> -   this->R0 = retype(brw_vec8_grf(reg++, 0), BRW_REGISTER_TYPE_UW);
> >> -   this->R1 = retype(brw_vec8_grf(reg++, 0), BRW_REGISTER_TYPE_UW);
> >> +   int reg = prog_data.first_curbe_grf;
> > 
> > This line doesn't make sense to me.  This function is what sets
> > prog_data.first_curbe_grf.  Presumably you want:
> > 
> > /* Reserve space for g0 and g1, which contain the thread payload. */
> > int reg = 2;
> > 
> > Otherwise, I think the line below will change clear_rgba to g0 rather
> > than g2, which is probably not what you want.  Maybe I'm misreading.
> 
> Indeed, I can't read, sorry.  You changed it so that compile() sets
> first_curbe_grf, and alloc_regs() uses it.  So your patch is correct.
> 
> Still, I think it would probably be nicer to keep it all contained in
> alloc_regs().  It's such a tiny function though...

And thrown away when the replicated path gets converted. I thought it would be
simpler to move the assignment to the final location already here.

> 
> > 
> >> -   prog_data.first_curbe_grf = reg;
> >>     clear_rgba = retype(brw_vec4_grf(reg++, 0), BRW_REGISTER_TYPE_F);
> >>     reg += BRW_BLORP_NUM_PUSH_CONST_REGS;
> >>  
> >> @@ -384,6 +373,8 @@ brw_blorp_const_color_program::compile(struct brw_context *brw,
> >>     /* Set up prog_data */
> >>     memset(&prog_data, 0, sizeof(prog_data));
> >>     prog_data.persample_msaa_dispatch = false;
> >> +   /* R0-1: masks, pixel X/Y coordinates. */
> >> +   prog_data.first_curbe_grf = 2;
> >>  
> >>     alloc_regs();
> > 
>