[Mesa-dev] [PATCH v2 2/6] i965/vec4: Remove checks for reladdr when checking for spillable registers

[Iago Toral]

On Thu, 2015-07-30 at 16:27 +0300, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral at igalia.com> writes:
> 
> > In theory, GRF array access should have been moved to scratch by the time
> > we got here, so this should never happen. A full piglit run forcing
> > spilling of all registers seems to confirm this. The FS backend
> > does not seem to check for this either.
> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> > index a9bf0d8..cff5406 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> > @@ -282,15 +282,13 @@ vec4_visitor::evaluate_spill_costs(float *spill_costs, bool *no_spill)
> >        for (unsigned int i = 0; i < 3; i++) {
> >  	 if (inst->src[i].file == GRF) {
> >  	    spill_costs[inst->src[i].reg] += loop_scale;
> > -            if (inst->src[i].reladdr)
> > -               no_spill[inst->src[i].reg] = true;
> > +	    assert(!inst->src[i].reladdr);
> >  	 }
> >        }
> >  
> >        if (inst->dst.file == GRF) {
> >  	 spill_costs[inst->dst.reg] += loop_scale;
> > -         if (inst->dst.reladdr)
> > -            no_spill[inst->dst.reg] = true;
> 
> I guess the intention behind these checks was to support unlowered
> indirect addressing of GRFs sometime in the future.  Is there some
> reason why you want to remove them?

The only reason was that AFAICT we never let indirect addressing of GRFs
get here, so this code is checking for a case that can't happen at the
moment and I figured that maybe some forgot to just remove it. But I am
okay with it if you think this can serve a purpose later on, I just
thought this was dead code.

> That said, I don't think there would be any reason to force indirectly
> addressed VGRFs not to spill even if we implemented them, because if the
> VGRF is larger than one physical register it will already have been
> marked as no-spill by the previous loop at the top of this function, and
> if they are exactly one physical register the reladdr value is
> guaranteed to be zero so the indirect addressing is a no-op.
> 
> How about we get rid of these checks as you have done, but leave no
> assertions behind?

Ben was working on trying to enable spilling of registers with size > 1,
he abandoned that idea for now but I figure that he or someone else may
want to do that at some point... so all things considered, maybe we
should just drop this patch?

> > +	 assert(!inst->dst.reladdr);
> >        }
> >  
> >        switch (inst->opcode) {
> > -- 
> > 1.9.1