[Mesa-dev] [PATCH v6 2/6] mesa/st: glsl_to_tgsi: implement new temporary register lifetime tracker

[Gert Wollny]

Just a few comments while updating the patch: 

> One more comment about the debug_log: as written, it will getenv 
> every time. It would be better to use the approach taken by e.g. 
> should_clone_nir() in compiler/nir/nir.h. There's no reason to have 
> a whole class for this.

Actually, debug_log is implemented as a singleton, and the constructor
that calles getenv is only called once, but yes, there is no need to
make it a class. 

> 
> > +
> > +         assert(num_inst_src_regs(inst) == 1);
> > +         const st_src_reg& src = inst->src[0];
> > +         if (src.file == PROGRAM_TEMPORARY)
> > +            acc[src.index].record_read(line, switch_scope,
> > src.swizzle);
> 
> I think this read (and the one of the switch_register) should both be
> on  the line of the SWITCH statement, so at the beginning of the
> switch_scope.
> 
> What you're doing is only more conservative, and I don't mind that
> much, except that at least it should be possible to remove the 
> set_switch_register and friends, since a read of the switch register 
> need not be recorded for every CASE statement.

Since SWITCH currently is not really generated it is difficult to test
what byte code would actually be later generated, but I figured that in
the worst case the SWITCH would translate to an IF-chain, in which case
for each CASE statement  one would actually need to read both, the
SWITCH value and the CASE value. 

> The two branches are inconsistent about where the new scope ends. 
> Overall, I think this could be simplified:
> 
>    prog_scope *switch_scope =
>       cur_scope->type() == switch_body ? cur_scope : cur_scope-
> >parent();
>    assert(switch_scope->type() == switch_body);
> 
>    prog_scope *scope = scopes.create(...);
> 
>    if (cur_scope != switch_body && cur_scope->end() == -1)
>       scope->set_previous_case_scope(cur_scope);
>    cur_scope = scope;
> 
> For that matter, I'm not sure the previous_case_scope handling is 
> necessary or whether it's correct at all. Do
I don't think it is necessary, and I will check whether it breaks
something.

>  you really need the end to overlap the next scope on fall-through?
> And if yes, what happens if have multiple fall-throughs in a row?
I think that this case is handled consistently. 

Anyway, I will contemplate about whether it makes sense to keep this
behavior, and I actually lean towards removing it. 

> +
> > +void temp_access::update_access_mask(int mask)
> > +{
> > +   if (!first_access_mask) {
> > +      first_access_mask = mask;
> > +   } else {
> > +      needs_component_tracking |= (first_access_mask != mask);
> > +   }
> > +   summary_access_mask |= mask;
> 
> This can be simplified to:
> 
>     if (mask != access_mask)
>        needs_component_tracking = true;
>     access_mask |= mask;
> 
> saving one member variable.

Actually, it must be 

    if (access_mask && (mask != access_mask))
       
needs_component_tracking = true;
        ... 
    
otherwise needs_component_tracking would always be set to true. 

> 
> > +    * the life time to [lr,lr), so it can me merged "away"
> > +    */
> > +   if (last_write < 0)
> > +      return make_lifetime(last_read, last_read);
> 
> Ah I see. I think it'd be better to return (-1, -1) here and handle
> that case when merging the lifetimes. It's not more code, since it
> allows you  to merge the first two case in this function, and it
> avoids an overly  conservative extension of the register lifetime in
> the unlikely case where a component is read from outside the scope of
> the other components.

My thinking was to completely ignore unused registers, and use read-
only registers for renaming, but now I checked that a register that is
only read is actually eliminated in 

glsl_to_tgsi_visitor::renumber_registers

so no need to bother distinguishing the two cases. 

Best, 
Gert