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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev