<div dir="ltr">On 20 February 2013 12:25, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>> writes:<br>
<br>
> From: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
><br>
> Previously, we had separate constructors for one, two, and four operand<br>
> expressions.  This patch consolidates them into a single constructor<br>
> which uses NULL default parameters.<br>
><br>
> The unary and binary operator constructors had assertions to verify that<br>
> the caller supplied the correct number of operands for the expression,<br>
> but the four-operand version did not.  Since get_num_operands for<br>
> ir_quadop_vector returns the number of vector_elements, we can safely<br>
> add that without breaking the semantics of ir_quadop_vector.<br>
><br>
> This also paves the way for expressions with three operands.  Currently,<br>
> none can be constructed since get_num_operands() never returns 3.<br>
><br>
> Reviewed-by: Matt Turner <<a href="mailto:mattst88@gmail.com">mattst88@gmail.com</a>><br>
> ---<br>
>  src/glsl/ir.cpp |   34 ++++++----------------------------<br>
>  src/glsl/ir.h   |   13 ++++---------<br>
>  2 files changed, 10 insertions(+), 37 deletions(-)<br>
><br>
> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp<br>
> index 954995d..4ccdc42 100644<br>
> --- a/src/glsl/ir.cpp<br>
> +++ b/src/glsl/ir.cpp<br>
</div><div class="im">> @@ -234,6 +206,12 @@ ir_expression::ir_expression(int op, const struct glsl_type *type,<br>
>     this->operands[1] = op1;<br>
>     this->operands[2] = op2;<br>
>     this->operands[3] = op3;<br>
> +#ifndef NDEBUG<br>
> +   int num_operands = get_num_operands(this->operation);<br>
> +   for (int i = num_operands; i < 4; i++) {<br>
> +      assert(this->operands[i] == NULL);<br>
> +   }<br>
> +#endif<br>
<br>
</div>Ugh, I don't like seeing #ifdefs for debug around when it's avoidable --<br>
we should be able to rely on this stuff getting optimized out.  Except<br>
that the optimizer doesn't get to know that get_num_operands() has no<br>
side effects, unless we annotate it with __attribute__((pure)).  I guess<br>
I'm fine either way -- this, or adding the attribute.<br></blockquote><div><br></div><div>In i965 I would be a fan of adding the attribute, but since this code is in glsl, it needs to be buildable using MSVC, which does not support __attribute__ declarations.<br>
<br></div><div>Two other options you might want to consider: (1) move the call to get_num_operands() inside the assert, like this:<br></div><div><br></div><div>for (int i = 0; i < 4; i++) {<br></div><div>   assert(i < get_num_operands(this->operation) || this->operands[i] == NULL);<br>
}<br><br>It'll be slightly 
slower on debug builds but I doubt it'll be slow enough to cause concern.<br><br></div><div>(2) Alternatively, you could make get_num_operands() an inline function declared in ir.h.  Then both MSVC and gcc should be smart enough to optimize away the debug check without needing an #ifdef or an attribute declaration to help them out.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
So, other than fixing the added tabs in new lines of some patches<br>
(particularly #6), patches 1 and 3-9 are<br>
<br>
Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>><br>
<br>_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
<br></blockquote></div><br></div></div>