On 20 February 2013 12:25, Eric Anholt <e...@anholt.net> wrote:
> Matt Turner <matts...@gmail.com> writes:
>
> > From: Kenneth Graunke <kenn...@whitecape.org>
> >
> > Previously, we had separate constructors for one, two, and four operand
> > expressions. This patch consolidates them into a single constructor
> > which uses NULL default parameters.
> >
> > The unary and binary operator constructors had assertions to verify that
> > the caller supplied the correct number of operands for the expression,
> > but the four-operand version did not. Since get_num_operands for
> > ir_quadop_vector returns the number of vector_elements, we can safely
> > add that without breaking the semantics of ir_quadop_vector.
> >
> > This also paves the way for expressions with three operands. Currently,
> > none can be constructed since get_num_operands() never returns 3.
> >
> > Reviewed-by: Matt Turner <matts...@gmail.com>
> > ---
> > src/glsl/ir.cpp | 34 ++++++----------------------------
> > src/glsl/ir.h | 13 ++++---------
> > 2 files changed, 10 insertions(+), 37 deletions(-)
> >
> > diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
> > index 954995d..4ccdc42 100644
> > --- a/src/glsl/ir.cpp
> > +++ b/src/glsl/ir.cpp
> > @@ -234,6 +206,12 @@ ir_expression::ir_expression(int op, const struct
> glsl_type *type,
> > this->operands[1] = op1;
> > this->operands[2] = op2;
> > this->operands[3] = op3;
> > +#ifndef NDEBUG
> > + int num_operands = get_num_operands(this->operation);
> > + for (int i = num_operands; i < 4; i++) {
> > + assert(this->operands[i] == NULL);
> > + }
> > +#endif
>
> Ugh, I don't like seeing #ifdefs for debug around when it's avoidable --
> we should be able to rely on this stuff getting optimized out. Except
> that the optimizer doesn't get to know that get_num_operands() has no
> side effects, unless we annotate it with __attribute__((pure)). I guess
> I'm fine either way -- this, or adding the attribute.
>
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.
Two other options you might want to consider: (1) move the call to
get_num_operands() inside the assert, like this:
for (int i = 0; i < 4; i++) {
assert(i < get_num_operands(this->operation) || this->operands[i] ==
NULL);
}
It'll be slightly slower on debug builds but I doubt it'll be slow enough
to cause concern.
(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.
>
> So, other than fixing the added tabs in new lines of some patches
> (particularly #6), patches 1 and 3-9 are
>
> Reviewed-by: Eric Anholt <e...@anholt.net>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
