----- Original Message -----
> On Mon, Jun 24, 2013 at 6:12 PM, Jose Fonseca <jfons...@vmware.com> wrote:
> >
> >
> > ----- Original Message -----
> >> ---
> >> src/gallium/auxiliary/util/u_format.c | 14 ++++++++++++++
> >> src/gallium/auxiliary/util/u_format.h | 3 +++
> >> src/mesa/state_tracker/st_cb_drawpixels.c | 6 ++++++
> >> 3 files changed, 23 insertions(+)
> >>
> >> diff --git a/src/gallium/auxiliary/util/u_format.c
> >> b/src/gallium/auxiliary/util/u_format.c
> >> index 9bdc2ea..b70c108 100644
> >> --- a/src/gallium/auxiliary/util/u_format.c
> >> +++ b/src/gallium/auxiliary/util/u_format.c
> >> @@ -131,6 +131,20 @@ util_format_is_pure_uint(enum pipe_format format)
> >> return (desc->channel[i].type == UTIL_FORMAT_TYPE_UNSIGNED &&
> >> desc->channel[i].pure_integer) ? TRUE : FALSE;
> >> }
> >>
> >> +boolean
> >> +util_format_is_snorm(enum pipe_format format)
> >> +{
> >> + const struct util_format_description *desc =
> >> util_format_description(format);
> >> + int i;
> >> +
> >> + i = util_format_get_first_non_void_channel(format);
> >> + if (i == -1)
> >> + return FALSE;
> >> +
> >> + return desc->channel[i].type == UTIL_FORMAT_TYPE_SIGNED &&
> >> + !desc->channel[i].pure_integer &&
> >> + desc->channel[i].normalized;
> >> +}
> >
> > This will give wrong results for mixed formats -- containing a mixture of
> > SIGNED and UNSIGNED normalized types. You can avoid that by adding this
> > statement
> >
> > if (desc->is_mixed) return FALSE at the start.
> >
> > I think a comment would be good too -- something like "Returns true if all
> > non-void channels are normalized signed."
> >
> > This is not your fault, but it is disappointing to see the proliferation of
> > redundant util_format_description() calls in these helpers.
> > util_format_description is not cost free, and st_CopyPixels and friends
> > keep calling it over and over again. I don't think it is hard to call
> > util_format_description() once, and then pass the format_desc pointers
> > around, but once we start on the other route is hard to back out. But as
> > they say -- when you're in a hole, the first thing to do is stop digging
> > --, so I think we should just stop adding new helper functions to util
> > u_format that take enum format instead of util_format_description.
>
> I hestiate to say this as no doubt I'll be wrong, but if they are all
> inline helpers, the compiler should figure it out and collapse the
> lookups. Of course I believe compilers should do lots of things...
It's not totally wrong what you say.
Ordinarily, a C compiler has no way to know that each call
util_format_description() with same arguments the same result.
That is, it has no way to know that that util_format_description doesn't refer
and modify other global state like:
const struct util_format_description *
util_format_description(format) {
static const struct util_format_description * last_format_description =
&my_table;
return ++last_format_desciption;
}
Unless,
- unless the compiler is using whole-program-optimzation /
link-time-optimization
- the implementation of util_format_description() is a header -- but that will
make compilation time slow.
- there is some attribute to tell the compiler that a function has no side
effects.
And I checked http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html and
apparently there is actually some gcc attributes that could help here:
-- __attribute__((const))
-- __attribute__((pure))
But support with other compilers varies --
http://stackoverflow.com/questions/2798188/pure-const-function-attributes-in-different-compilers
. declspec(noalias) comes close,
http://msdn.microsoft.com/en-us/library/k649tyc7.aspx , but not sure if it
works. It might be worth a try though.
Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
