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

[Gert Wollny]

Thanks for your comments 


Am Montag, den 26.06.2017, 14:52 +0200 schrieb Nicolai Hähnle:
> Thanks for the update. First off, you're still not tracking
> individual 
> components, but that's absolute necessary. Think:
> 
> BGNLOOP
>    MOV TEMP[1].x, ...
> 
>    UIF ...
>      MOV TEMP[1].y, ...
>    ENDIF
> 
>    use TEMP[1].y
> ENDLOOP

Added test case and implemented. 



> +
> > +enum e_scope_type {
> 
> Please drop the "e_" prefix here and below, we don't usually do that.

done. Do you also mean below? I usually do this to avoid name clashes
...

> 
> > +         /* Switch cases and default are handled at the same
> > nesting level
> > +          * like their enclosing switch */
> 
> Why? It seems surprising to mess with the invariant that 
> parent->nesting_depth() == nesting_depth() - 1.

I'll change this.  

> +      case TGSI_OPCODE_CONT: {
> > +         cur_scope->set_continue_line(line);
> 
> I'm still frankly confused about the way you choose to handle
> BRK/CONT in loops, and suspect you're doing it wrong. At the very
> least, having a function called "set_continue_line" be called for a
> BRK is bad naming.

Well, I'll also changed this. In any case, handling continue like break
only means that the required lifetime of some temporaries would be
overestimated, which is not a big problem (as compared to
underestimating it). 



> > +         e_acc_type write_type = inst->op == TGSI_OPCODE_UCMP ?
> 
> Despite the opcode being called "Integer Conditional Move", it does 
> write to dst unconditionally. It should probably have been called 
> "select" or something like that.

I'm aware of that, the reason why I'm tracking this explicitly is
because considering this line with TEMP[5] never written before: 

  UCMP TEMP[5], IN[0], TEMP[5], In[1]

TEMP[5] can be well defined after the write, so I have to take the
write into account as a dominant write. On the other hand 

 MOV TEMP[5], TEMP[5]

means that since the read from TEMP[1] is always undefined, the write
to TEMP[1] also is and I only have to make sure that TEMP[5] is not
merged with another register that would then be overwritten. Actually,
I would hope that the dead code elimination removes such statements.


[...] 

+
> > +      last_write = line;
> > +
> > + /* If no first write is assigned check whether we deal with a
> > case where the temp is read and written in the same instructions,
> > because then it is not a dominant write, it may even be undefined.
> > Hence postpone the assignment if the first write, only mark that
> > the register was  written at all by remembering a scope */
> > +
> 

> Also, I think the comment is wrong. It should count as a dominating 
> write even if there's a read on the same line. So the special
> handling here is wrong.
This is exactly the case that I commented above, i.e. in the cases when
it is undefined behavior should I keep the register alive? I opted for
no. 


> 
> > +      if (first_dominant_write < 0) {
> > +
> > +          if (line != last_read || (rw ==
> > acc_write_cond_from_self))
> > +             first_dominant_write = line;
> > +
> > +          first_write_scope = scope;
> 
> Should this be renamed to first_dominant_write_scope?
okay. 

> > +      }
> > +
> > +      if (scope->is_conditional() && scope->in_loop())
> > +         keep_for_full_loop = true;
> 
> This is only necessary as long as we don't have a dominant write yet,
> right?

I have a test case for this, if you have the dominant write in a loop
within conditional within loop then the propagation must be done when
moving up the scopes. 

>> +#include <iostream>
> > +using std::cerr;
> 
> Always put includes and usings at the top of a source file.
Sorry, that was supposed to be just a quick check that I wanted to
remove later, but forgot to do so. 



> > +
> > +
> > +   /* Undefined behaviour: read and write in the same instruction
> > +    * but never written elsewhere. Since it is written, we need to
> > +    * keep it nevertheless.
> 
> This doesn't actually need to be undefined behavior, depending on
> the  instruction. It's likely to be dead code though.

CMIIW, but with the exception of UCMP I don't see a case where this is
not undefined behavior, and UCMP is handled differently. 

> Also, the actual code below doesn't reflect the comment.
I'll try to improve the comment. 

> Don't you have to expand this to the extend of the outermost loop?
Why? if it is undefined I don't need to keep the register around for
more that the instructions where it is written (hence the last_write
+1).


> > +   /* Evaluate the scope that is shared by all three, first write,
> > and
> > +    * first (conditional) read before write and last read. */
> 
> What's a conditional read, and why does it matter?

An unconditional read before a first dominant write is undefined. A
conditional read before the dominant write (in a loop) is very likely
well defined and for that reason we have to take it into account. e.g. 

Y=0
I=0
BGNLOOP 
  IF i > 1
    ADD Y, X, Y
  ENDIF 
  X = 1
  I = I + 1
  IF I > 5
    BRK
  ENDIF 
ENDLOOP 
OUT = Y

Here access to X is always well defined. With 

Y=0
I=0
BGNLOOP 
  ADD Y, X, Y
  X = 1
  I = I +1
  IF I > 5
    BRK
  ENDIF 
ENDLOOP 
OUT = Y

The read access to X in line 4 is undefined in the first round, 
and hence Y and OUT are undefined. Hence, there is no point in keeping 
X alive for the full loop like in the above case. 

If, on the other hand we have 

Y=0
I=0
BGNLOOP 
  UCMP Y, I, IN0, X
  X = 1
  I = I +1
  IF I > 5
    BRK
 ENDIF 
ENDLOOP 
OUT = Y

then X must be kept alive for the whole loop. 

> > +
> > +   /* Here we are at the same scope, all is resolved */
> > +   return make_lifetime(first_dominant_write, last_read);
> 
> I suspect that there are a lot of logical cleanups and
> simplifications that you can achieve in this function but sticking to
> a straight story of what every variable really means.
> 
> But please, first address the issue of multiple components and all
> the style issues, then we can see what to about this.

Okay. Although I think that there are not many simplifications possible
that don't sacrifice a close-to-minimal estimated life time.

Best, 
Gert