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

[Gert Wollny]

Hi, 

> > >+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.
> 
> How? Perhaps your variable names stand to be improved. We try to av
oid ineffective abstractions in Mesa.

Since all of my references to the scopes where using this type, I was
able to easily switch to pointers, which also enabled me to eliminate
the use of an explicit stack.


> > I think with the new design that uses pointers into a pre-allocated
> > array the methods must stay where they are.
> 
> No, they really don't. You're thinking of scopes etc. as objects. If
> you  just thought of them as data, as IMO you should, it would feel
> natural to move the methods into the main class and eliminate those
> back-pointers.
Maybe, I did C++ and OOP for too much time, I kind of like the object
based approach, because it helps me better to focuse when working with
the code.

> > > > TGSI loops are always infinite-loops. So actually, every loop
> > > > should 
> have a BRK somewhere. But the point remains that CONT itself cannot
> skip code indefinitely, because there's no breaking out of the loop
> in BGNLOOP.

I see your point, I will think about how this requires different
handling in the life-time analysis, and I also think that handling this
incorrectly might have clobbered my algorithm, i.e. piglit and
compiling shader-db didn't show any problems, and that's what I tested
exhaustively for the last two submissions, but the GpuTest piano
benchmark, that worked  with my first version, stopped to work again.
Now I'm redoing everything from version one incorporating what I have
learned step by step. 

With that interpretation, [a,b] and [b,c] are  genuinely disjoint,
and it allows merging TEMP[1] and TEMP[2] in
> 
>    ADD TEMP[1], ..., ....
>    ADD TEMP[2], TEMP[1], ...
> 
> which is good.
Somehow I already understood that this is how it works. 


> I think this all makes sense, but you should be more explicit about
> it  and make sure all the variable names are consistent with that
> convention.
Okay, I will strife for it, thanks again for your very helpful
comments, 

Gert