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 <kenn...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
