Add Variable Packing checks to ANGLE

Add Variable Packing checks to ANGLE

[gmanchromium, 2012-10-12 01:22:05]

Al or Alok, could you please review. Thanks

For 
http://code.google.com/p/angleproject/issues/detail?id=373

[kbr1, 2012-10-12 02:01:25]

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp...
src/compiler/VariablePacker.cpp:144: ++topNonEmptyRow_);
Given this definition, would topNonEmptyRow_ be better called topNonFullRow_?

[Alok Priyadarshi, 2012-10-15 21:32:48]

https://codereview.appspot.com/6652051/diff/1/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

https://codereview.appspot.com/6652051/diff/1/src/compiler/Compiler.cpp#newco...
src/compiler/Compiler.cpp:323: return
packer.CheckVariablesWithinPackingLimits(maxUniformVectors, uniforms);
Does this mean SH_ENFORCE_PACKING_RESTRICTIONS relies on SH_ATTRIBUTES_UNIFORMS
being defined?

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp...
src/compiler/VariablePacker.cpp:280: std::sort(variables.begin(),
variables.end(), TVariableInfoComparer());
can you sort the variable-list outside this function to avoid copying? This
function could assume that the list is already sorted.

[gmanchromium, 2012-10-16 22:20:32]

I updated the code to make it simpler and easier to understand I think.

https://codereview.appspot.com/6652051/diff/1/src/compiler/Compiler.cpp
File src/compiler/Compiler.cpp (right):

https://codereview.appspot.com/6652051/diff/1/src/compiler/Compiler.cpp#newco...
src/compiler/Compiler.cpp:323: return
packer.CheckVariablesWithinPackingLimits(maxUniformVectors, uniforms);
On 2012/10/15 21:32:49, Alok Priyadarshi wrote:
> Does this mean SH_ENFORCE_PACKING_RESTRICTIONS relies on
SH_ATTRIBUTES_UNIFORMS
> being defined?

Done.

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp...
src/compiler/VariablePacker.cpp:144: ++topNonEmptyRow_);
On 2012/10/12 02:01:25, kbr1 wrote:
> Given this definition, would topNonEmptyRow_ be better called topNonFullRow_?

Done.

https://codereview.appspot.com/6652051/diff/1/src/compiler/VariablePacker.cpp...
src/compiler/VariablePacker.cpp:280: std::sort(variables.begin(),
variables.end(), TVariableInfoComparer());
On 2012/10/15 21:32:49, Alok Priyadarshi wrote:
> can you sort the variable-list outside this function to avoid copying? This
> function could assume that the list is already sorted.

It didn't seem appropriate to sort this outside the function as that makes it
un-encapsulated. AFAIK there might be code in the compiler that is indexing the
TVariableInfoList and my sorting it will break the indices (I'm not saying there
is code like that I'm saying that as it is now I don't have to care if there is
or not so I don't break anything)

What do you think?

[kbr1, 2012-10-17 20:18:38]

On 2012/10/16 22:20:32, gmanchromium wrote:
> It didn't seem appropriate to sort this outside the function as that makes it
> un-encapsulated. AFAIK there might be code in the compiler that is indexing
the
> TVariableInfoList and my sorting it will break the indices (I'm not saying
there
> is code like that I'm saying that as it is now I don't have to care if there
is
> or not so I don't break anything)

Personally I definitely agree with keeping this packing algorithm self-contained
and separable from the rest of the compiler. It's a little risky (may break
content, may incorrectly flag valid shaders as not compilable) so it's strongly
desirable to have it under a flag and keep it completely self-contained.

[kbr1, 2012-10-17 20:39:16]

Looks good overall; definitely seems easier to read than earlier version. One
question.

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker....
src/compiler/VariablePacker.cpp:252: return false;
Does this implement the rule "For 2 component variables, when there are no spare
rows, the strategy is switched..."?

[gmanchromium, 2012-10-17 20:51:27]

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker....
src/compiler/VariablePacker.cpp:252: return false;
On 2012/10/17 20:39:16, kbr1 wrote:
> Does this implement the rule "For 2 component variables, when there are no
spare
> rows, the strategy is switched..."?

yes. The code basically fills in 4 and 3 column uniforms. That leaves only the
rows below that to fit 2 column uniforms. At that point you can basically
consider it 2 columns of 2 column uniforms 

4444
4444
4444
333.
333.
333.
aabb
aabb
aabb
aabb

In this diagram 4444 is full of 4 column uniforms. 333 is the 3 column uniforms.
aa is 1 empty column of 2 column free space. bb is 1 empty column of 2 column
free space. 

The rule is fill column 'aa' from the top down. If it's full fill column 'bb'
from the bottom up. (that's strategy #2) 

But all I need to do to figure that out is allocate to either column. Column aa
first, column bb if there's no room in aa. If both columns are full then there
is no room so fail.

If it didn't fail then, how ever big the columns were, for the aa column mark
the top rows as used. For column bb mark the bottom used rows as used.

[kbr1, 2012-10-17 21:09:04]

The implementation of the algorithm looks great. LGTM

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker.cpp
File src/compiler/VariablePacker.cpp (right):

https://codereview.appspot.com/6652051/diff/9002/src/compiler/VariablePacker....
src/compiler/VariablePacker.cpp:252: return false;
On 2012/10/17 20:51:27, gmanchromium wrote:
> On 2012/10/17 20:39:16, kbr1 wrote:
> > Does this implement the rule "For 2 component variables, when there are no
> spare
> > rows, the strategy is switched..."?
> 
> yes. The code basically fills in 4 and 3 column uniforms. That leaves only the
> rows below that to fit 2 column uniforms. At that point you can basically
> consider it 2 columns of 2 column uniforms 
> 
> 4444
> 4444
> 4444
> 333.
> 333.
> 333.
> aabb
> aabb
> aabb
> aabb
> 
> In this diagram 4444 is full of 4 column uniforms. 333 is the 3 column
uniforms.
> aa is 1 empty column of 2 column free space. bb is 1 empty column of 2 column
> free space. 
> 
> The rule is fill column 'aa' from the top down. If it's full fill column 'bb'
> from the bottom up. (that's strategy #2) 
> 
> But all I need to do to figure that out is allocate to either column. Column
aa
> first, column bb if there's no room in aa. If both columns are full then there
> is no room so fail.
> 
> If it didn't fail then, how ever big the columns were, for the aa column mark
> the top rows as used. For column bb mark the bottom used rows as used.

Ah yes, thanks, I see now.

[Alok Priyadarshi, 2012-10-17 21:14:00]

lgtm