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

[Nicolai Hähnle]

On 17.07.2017 04:32, Gert Wollny wrote:
> 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.

Ah yes, you're right, missed the static. Still would be clearer as not a 
class, I think.


>>> +
>>> +         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 only thing that actually implements TGSI_OPCODE_SWITCH is 
tgsi_exec.c, and it only reads the switch value when it encounters the 
SWITCH statement.

So I think you're right, it's good to have the reads on the CASE source 
on the line of the CASE, but I also think the read of the SWITCH source 
should only be one the SWITCH line. It simplifies the code here, and 
matches the only possible user.


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

Thanks.


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

Right, good point.


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

Great!

Cheers,
Nicolai

> 
> Best,
> Gert
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.