[Mesa-dev] [PATCH 5/5] Change ir_function_signature::constant_expression_value to run through the function.
Kenneth Graunke
kenneth at whitecape.org
Mon Apr 30 12:57:36 PDT 2012
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
More information about the mesa-dev
mailing list