Recursively write ConstantUnion to correctly construct structs. Also renamed ...

Recursively write ConstantUnion to correctly construct structs.

[Alok Priyadarshi, 2010-05-04 23:08:31]

Tips for quick code review: I have only changed code in
TOutputGLSL::writeConstantUnion and TOutputGLSL::visitConstantUnion. Rest of the
changes are just constUnion -> ConstantUnion name changes.

[kbr1, 2010-05-05 00:59:03]

LGTM, but is this the most general fix? Also, does the HLSL backend need to be
changed similarly?

[Alok Priyadarshi, 2010-05-05 02:00:44]

Another way to fix this would be to change reduction rules so that
struct-construction is not reduced to a ConstantUnion. I did not do
that because all the information needed to correctly write GLSL was
already available in the ConstantUnion. The only drawback to this
approach is that code for constructing structs is at two places. We
have the same problem for constructing GLSL vectors and matrices. If
we are willing to change the grammar I would fix both user-defined
structs and standard vectors and matrices. But I would prefer to not
change the grammar unless absolutely needed.

HLSL backend is writing a custom constructor for structs. But I guess
HLSL backend can also be written in a similar fashion.

On Tue, May 4, 2010 at 6:59 PM,  <kbr@chromium.org> wrote:
> LGTM, but is this the most general fix? Also, does the HLSL backend need
> to be changed similarly?
>
>
> http://codereview.appspot.com/1108041/show
>

[dgkoch, 2010-05-05 13:57:06]

Hi Alok,

We like the solution you used here.   We've tried the same approach in
OutputHLSL and it fixes the known issue we had while still passing the
conformance tests.

This does seem to be the most elegant solution.  Changing the constant
propagation and folding/reduction can be tricky and we'd also like to avoid
modifying the grammar unless absolutely necessary. 

The only problem with the current patch is that it renames constUnion to
ConstantUnion along with the functional change. This part of the patch does
affect OutputHLSL.h and .cpp and does in fact break the build.  I would prefer
if you did the renaming in a separate patch (and ensure that OutputHLSL still
compiles as part of this) and then have a separate patch to fix the actual
issue.  Then we can roll out the same fix in OutputHLSL.

Daniel

[Alok Priyadarshi, 2010-05-05 14:57:52]

Yes I should have made two separate patches. Renaming change is split into a
separate patch here: http://codereview.appspot.com/1106042/show

Renaming does not affect OutputHLSL.[h,cpp] because they do not explicitly refer
to ConstantUnion. I did a global search-replace and built the whole solution to
make sure.

[dgkoch, 2010-05-05 15:05:59]

Oops, I guess we don't directly reference that.

Looks good to me then (with the other changes factored out).

[kbr1, 2010-05-05 16:22:29]

On 2010/05/05 15:05:59, dgkoch wrote:
> Oops, I guess we don't directly reference that.
> 
> Looks good to me then (with the other changes factored out).

What happens if a struct is initialized with the contents of variables rather
than constants? For example:

vec3 tv = vec3(1.0, 0.0, 0.0); // Or some expression not easily reduced to a
constant
struct SStruct {
  float f;
  vec3 v;
};
SStruct s = SStruct(0.0, tv);

Is this legal? Will it be handled properly?

[Alok Priyadarshi, 2010-05-05 16:36:47]

Yes this gets handled properly with an EOpConstructStruct node. Thats what I
meant by struct-construction code at two places.