[Mesa-dev] [PATCH v2 2/3] mesa/st: glsl_to_tgsi Implement a new lifetime tracker for temporaries

[Gert Wollny]

Hello Nicolai, 


Am Sonntag, den 18.06.2017, 12:05 +0200 schrieb Nicolai Hähnle:
>   
> >   if HAVE_XLIB_GLX
> >   SUBDIRS += drivers/x11
> > @@ -101,7 +101,7 @@ AM_CFLAGS = \
> >   	$(VISIBILITY_CFLAGS) \
> >   	$(MSVC2013_COMPAT_CFLAGS)
> >   AM_CXXFLAGS = \
> > -	$(LLVM_CFLAGS) \
> > +        $(LLVM_CXXFLAGS) \
> 
> I kind of suspect that this might be a no-no. On the one hand it
> makes sense, because it makes the use of CXXFLAGS consistent, but
> it's a pretty significant build system change.
I understand that, but c++11 is just such an improvement. 

> 
> At least separate it out into its own patch.
Okay, 


> 
> 
> 
> >   	$(VISIBILITY_CXXFLAGS) \
> >   	$(MSVC2013_COMPAT_CXXFLAGS)
> >   
> > diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> > index 21f9167bda..a68e9d2afe 100644
> > --- a/src/mesa/Makefile.sources
> > +++ b/src/mesa/Makefile.sources
> > @@ -509,6 +509,8 @@ STATETRACKER_FILES = \
> >   	state_tracker/st_glsl_to_tgsi.h \
> >   	state_tracker/st_glsl_to_tgsi_private.cpp \
> >   	state_tracker/st_glsl_to_tgsi_private.h \
> > +        state_tracker/st_glsl_to_tgsi_temprename.cpp \
> > +	state_tracker/st_glsl_to_tgsi_temprename.h \
> 
> Inconsistent use of whitespace.
> 
> Then the whole patch needs to be re-arranged, i.e. the 
> st_glsl_to_tgsi_private stuff, 
This was a mistake when I was re-basing the patch. 

> and I agree with Emil that the tests  
> should be separated out into their own patch.
I'm currently working on this. 

> 
> BTW, you can and should use
> 
> git rebase -x "cd $builddir && make" $basecommit
> 
> to verify that you don't break the build in an intermediate patch.
Nice trick, thanks. 


> > +typedef int scope_idx;
> 
> Please remove this, it's an unnecessary and distracting abstraction
> that  doesn't gain you anything.
Actually, with the refactoring it did the last two days it helped a
lot. 


> I consider the scopes back-reference here and in temp_access to be
> bad  style. 
I already got rid of them. 

> Think less object- and more data-oriented, then you won't need them.
> Most of those helper functions should be members of 
> tgsi_temp_lifetime -- and the getters/setters can be removed
> entirely.  YAGNI.
I think with the new design that uses pointers into a pre-allocated
array the methods must stay where they are. 


> To first approximation, you should never use mutable. Those methods 
> should not be const to begin with.
This is something else I actually didn't like and corrected already.  

> > +scope_idx
> > +tgsi_temp_lifetime::make_scope(e_scope_type type, int id, int lvl,
> > +                               int s_begin)const
> > +{
> > +   int idx = scopes.size();
> > +   scopes.push_back(prog_scope(type, idx, id, lvl, s_begin,
> > scopes));
> > +   return idx;
> > +}
> 
> AFAICS this overload is only used once. Please remove it.
Okay. 

> Use C-style comments. 
Okay, 

> Also, this should probably have an assert, or can this really happen?
I don't think it can happen, an assert it is.


> +      case TGSI_OPCODE_IF:
> > +      case TGSI_OPCODE_UIF:{
> > +         if (inst->src[0].file == PROGRAM_TEMPORARY) {
> > +               acc[inst->src[0].index].append(line, acc_read,
> > current);
> > +         }
> > +         scope_idx scope = make_scope(current, sct_if, if_id,
> > nesting_lvl, line);
> 
> It's probably a good idea to have scopes start at line + 1.
> Otherwise, 
> it may be tempting to think that the read access of the IF condition 
> happens inside the IF scope, at least based on the line numbers.

I don't think that's a problem. At least I can't think of a test case
that would trigger a problem with that. 


> 
> > +      case TGSI_OPCODE_DEFAULT: {
> > +         auto scope_type = (inst->op == TGSI_OPCODE_CASE) ?
> > +                              sct_switch_case :
> > sct_switch_default;
> > +         if ( scopes[current].type() == sct_switch ) {
> 
> No spaces inside parenthesis (also elsewhere).
Okay. 

> > +            scope_stack.push(current);
> > +            current = make_scope(current, scope_type,
> > scopes[current].id(),
> > +                                 nesting_lvl, line);
> > +         }else{
> 
> Spaces around else (also elsewhere).
Okay. 

> 
> No comment between if- and else-block.
Okay. 

> 
> 
> > +            scopes[current].set_continue(current, line);
> 
> You do need to distinguish between break and continue (at least for 
> accuracy), because break allows you to skip over a piece of code 
> indefinitely while continue doesn't. I.e.:

I risk to differ, in the worst case a CONT can act like it would be a
BRK, and because I have to handle the worst case it doesn't make sense
to distinguish between the two. 


> 
>     BGNLOOP
>        ...
>           BRK/CONT
>        ...
>        MOV TEMP[i], ...
>        ...
>     ENDLOOP
> 
> If it's a CONT, then i is written unconditionally inside the loop. 
While one would normally expect this, it is not guarantied. Think of a
shader like this: 

... 
varying int a; 
varying float b; 

int f(int a, float b) {...}
for (int i = 0; i< n; ++i ) {
   if (a && f(i, b)) 
      continue; 
   ...
   x = 
}

It may not be the best style. but it is possible.


> +
> > +         for (unsigned j = 0; j < inst->tex_offset_num_offset;
> > j++) {
> > +            if (inst->tex_offsets[j].file == PROGRAM_TEMPORARY) {
> > +               acc[inst->tex_offsets[j].index].append(line,
> > acc_read, current);
> > +            }
> > +         }
> 
> You need to handle the reads before the writes. After all, you might
> have a
> 
>     ADD TEMP[i], TEMP[i], ...
> 
> which may have to register as an undefined read.
I will add a test case, and correct the code accordingly. 


> 
> 
> > +
> > +      } // end default
> > +      } // end switch (op)
> 
> No end-of-scope comments.
okay, 

> > 
> > +scope_idx
> > +prog_scope::get_parent_loop() const
> 
> Based on what it does, this should be renamed to something like 
> get_innermost_loop.
Okay. 

> 
> > +void temp_access::append(int line, e_acc_type acc, scope_idx idx)
> > +{
> > +   last_write = line;
> 
> Looks like last_write should be called last_access.

Initially I thought the same, but a last read is also (most of the
time) the last access, and it is handled differently then a write past
the last read, so i think that last_write catches it better. 


> > +   if (acc == acc_read) {
> > +      last_read = line;
> > +      last_read_scope = idx;
> > +      if (undefined_read > line) {
> > +         undefined_read = line;
> > +         undefined_read_scope = idx;
> > +      }
> 
> The way you're using it this is effectively just first_read, not 
> undefined_read.

Indeed. 

> 
> 
> > +   } else {
> > +      if (first_write == -1) {
> > +         first_write = line;
> > +         first_write_scope = idx;
> > +
> > +         // we write in an if-branch
> > +         auto fw_ifthen_scope = scopes[idx].in_ifelse();
> > +         if ((fw_ifthen_scope >= 0) &&
> > scopes[fw_ifthen_scope].in_loop()) {
> > +            // value not always written, in loops we must keep it
> > +            keep_for_full_loop = true;
> > +         } else {
> > +            // same thing for switch-case
> > +            auto fw_switch_scope = scopes[idx].in_switchcase();
> > +            if (fw_switch_scope >= 0 &&
> > scopes[fw_switch_scope].in_loop()) {
> > +               keep_for_full_loop = true;
> > +            }
> > +         }
> 
> Simplify this by using an in_conditional() instead of in_ifelse + 
> in_switchcase().
I was thinking about this, but I also thought that when I want to track
all code path later then I will have to distinguish the two again, but,
on the other hand, adding a method that combines the two is a no-
brainer. 


> 
> > +
> > +   /* Only written to, just make sure that renaming
> > +    * doesn't reuse this register too early (corner
> > +    * case is the one opcode with two destinations) */
> > +   if (last_read_scope < 0) {
> > +      return make_pair(first_write, first_write + 1);
> 
> What if there are multiple writes to the temporary?
Good catch! Next text case :) 


> > +   /* propagate lifetime also if there was a continue/break
> > +    * in a loop and the write was after it (so it constitutes
> > +    * a conditional write */
> 
> It's only conditional if there was a break. Continue doesn't make it 
> conditional.
I think I made my argument. 

> +   /* propagate lifetimes before moving to upper scopes */
> > +   if ((scopes[first_write_scope].type() == sct_loop) &&
> > +       (keep_for_full_loop || (undefined_read < first_write))) {
> > +      first_write = scopes[first_write_scope].begin();
> > +      int lr = scopes[first_write_scope].end();
> > +      if (last_read < lr)
> > +         last_read = lr;
> > +   }
> 
> What about:
> 
>     BGNLOOP
>        ...
>        BGNLOOP
>           IF ...
>              read TEMP[i]
>           ENDIF
>           write TEMP[i]
>        ENDLOOP
>        ...
>     ENDLOOP
> 
> In this case, you don't set keep_for_full_loop, yet the lifetime
> must extend for the entire _outer_ loop.
Another test case that needs to be added :) 

> 
> I see two related problems in the code:
> 
> 1. In this case, keep_for_full_loop needs to be set to true (because 
> overwriting first_write in the code above means that the related
> check below won't fire).

> 
> 2. target_level is set to the inner loop even though it really needs
> to be set to the outer loop.
Indeed. 

Thanks a lot, 
Gert