[Mesa-dev] [PATCH 1/9] glsl: Consolidate ir_expression constructors that use explicit types.

[Paul Berry]

On 20 February 2013 12:25, Eric Anholt <eric at anholt.net> wrote:

> Matt Turner <mattst88 at gmail.com> writes:
>
> > From: Kenneth Graunke <kenneth at 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 <mattst88 at 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 <eric at anholt.net>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130220/afdcbb31/attachment.html>