[Mesa-dev] [PATCH 5/5] Change ir_function_signature::constant_expression_value to run through the function.

[Kenneth Graunke]

On 04/27/2012 01:28 AM, Olivier Galibert wrote:
> That removes code duplication with
> ir_expression::constant_expression_value and builtins/ir/*.
[snip]
> +	/*
> +	 * (assign [condition] (write-mask) (ref) (value))
> +	 *
> +	 * Do the assignement.  Bail out if a condition is present.
> +	 */
> +      case ir_type_assignment: {
> +	 ir_assignment *asg = inst->as_assignment();
> +	 if(asg->condition) {
> +	    result = NULL;
> +	    goto done;

I'm pretty sure this will break the mix(vec, vec, bvec) variants, which 
I implemented via conditional assignment.  It should be easy to support, 
though: just get the constant value of asg->condition, if true do the 
assignment, if not don't.

[snip]
> +
> +	 /* (return (expression))
> +	  *
> +	  * End of the line, compute the result and exit.
> +	  */
> +      case ir_type_return:
> +	 result = inst->as_return()->value->constant_expression_value(deref_hash);
> +	 goto done;
> +
> +	 /* Every other expression type, we drop out.  Maybe some
> +	    builtins will need jumps someday, but not yet. */
> +      default:
> +	 goto done;

Our implementations of faceforward() and refract() use the (if ...) 
construct.  I don't see that handled here, so I think they'll break too.

Other than the two missing features, I like this idea.  It does greatly 
simplify things going forward.  We'd talked about doing this, back in 
the day, but I punted on it, thinking it'd be more complicated than 
this.  Thanks for reworking this!

Patches 1-4 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Have you done a Piglit run on this to make sure that nothing regresses?

git clone git://anongit.freedesktop.org/piglit
cd piglit
cmake .
make -j5
mkdir results

./piglit-run.py tests/quick.tests results/before
./piglit-run.py tests/quick.tests results/after
rm -rf summary; ./piglit-summary.html summary results/before results/after

That will generate a nice html report in summary/changes.html that shows 
if you regressed anything.

Also: a couple general nits about coding style:

- Please put a space after if/switch:

    if (...) {
    }
    switch (...) {
    }

- Please use braces around loops that aren't a single line, i.e.

    for (i = 0; i < 4; i++) {
       if (...) {   // even though this is a single statement
       }
    }

    for (i = 0; i < 4; i++)
       offset += 3; // this is fine though

- Indentation is 3 spaces (typically using 8-space tabs).
   (It might be fine; some stuff was showing up oddly in the diff,
    but that tends to happen.)

Thanks again.  Once this supports conditional assignments and 
if-statements, I think it'll be good to go.

--Ken