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

[Gert Wollny]

Hello Nicolai, 

thanks again for your insightful comments, I am really on a steep
learning curve here :) 

I've pushed all my work on this to github, but since I mostly went on
refactoring in baby steps you might not want to look at the commits
separately. 

Am Mittwoch, den 28.06.2017, 10:37 +0200 schrieb Nicolai Hähnle:
> On 27.06.2017 11:32, Gert Wollny wrote:
> 
> Yes, I also mean below.
Okay, done. 

> 
> 
> [snip]
> > > +      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).
> 
> True, but it can also be confusing, so I think it's better to change.

Also done. I've also added a some more tests to check that break is
properly handled. These tests were missing when I prepared v3, and for
that reason I a green board on the tests and a failure with real world
applications. 

> 
> Hmm. I guess I'd have to look more carefully at how you do the
> lifetime calculation at the end. I wouldn't be surprised if what
> you're doing is correct, but I still think this is one of those
> instances where you're making your own life too difficult by not
> having your concepts clear.

No, I was not correct, not even by accident, and I now understand that
you are absolutely right about this. Namely 

  BGNLOOP 
     MUL TEMP[1], IN[0], TEMP[1]
     ... 
  ENDLOOP 

failed to properly extend to the loop. 


> 
> > 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.
> 
> Maybe you're referring to this example:
> 
> BGNLOOP
>    IF ...
>      BGNLOOP
>        MOV TEMP[0], ...
>      ENDLOOP
>    ENDIF
> ENDLOOP
> 
> In this case, the lifetime must span the entire outer loop.
Exactly. 

> However, consider this example, where you have an earlier dominating
> write:
> 
> BGNLOOP
>    MOV TEMP[0], ...
>    IF ...
>      BGNLOOP
>        MOV TEMP[0], ...
>      ENDLOOP
>    ENDIF
> ENDLOOP
> 
> Here, the lifetime must span only from the first MOV to the last use 
> inside the outer loop.
Indeed, and in my algorithm, the move within the IF statement is not
recorded as dominant. I added a specific test for this, but there was
no need to change the algorithm to make it pass. 


> The above is a perfect example of that. There is a check for whether 
> there has been a dominating write *right there*, and setting the 
> keep_for_full_loop is only necessary if there hasn't been a
> dominating 
> write before, so why is that not inside the dominating write check? 
At the point I was about to argue that it has to be done the way I do
it, and then I checked my latest code again, and saw that I had moved
that part out of the while loop :) 

I still have the break handling inside that while loop where I move the
dominant write scope up, and I do not yet see whether this could or
should also be resolved earlier. 


> It just reeks of coding by trial-and-error, and that's frankly not
> good enough.
Well, I wouldn't call it "coding by trial-and-error". As a fan of TDD
I look at it differently: Defining tests and making them pass is
initially the aim and every short cut is allowed, but then one must
refactor the code to improve it, and with the test cases in place this
refactoring can be done without the fear to break the code.


> Also, just a reminder of the issue of writemasks and tracking
> components separately.
This is already done. 

My next step will be to (re-)check formatting, white spaces etc. I
already did a lot of cleaning up, but I want to be sure nothing slips
through - and of course some benchmarking. 

many thanks again for your time, 
Gert