[Mesa-dev] [PATCH 8/8] mesa: Micro-optimize _mesa_is_valid_prim_mode

[Kenneth Graunke]

On Friday, December 19, 2014 02:20:59 PM Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
> 
> You would not believe the mess GCC 4.8.3 generated for the old
> switch-statement.
> 
> On Bay Trail-D using Fedora 20 compile flags (-m64 -O2 -mtune=generic
> for 64-bit and -m32 -march=i686 -mtune=atom for 32-bit), affects
> Gl32Batch7:
> 
> 32-bit: Difference at 95.0% confidence -0.37374% +/- 0.184057% (n=40)
> 64-bit: Difference at 95.0% confidence 0.966722% +/- 0.338442% (n=40)
> 
> The regression on 32-bit is odd.  Callgrind says the caller,
> _mesa_is_valid_prim_mode is faster.  Before it says 2,293,760
> cycles, and after it says 917,504.
> 
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---
>  src/mesa/main/api_validate.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
> index b882f0e..9c2e29e 100644
> --- a/src/mesa/main/api_validate.c
> +++ b/src/mesa/main/api_validate.c
> @@ -113,27 +113,21 @@ check_valid_to_render(struct gl_context *ctx, const char *function)
>  bool
>  _mesa_is_valid_prim_mode(struct gl_context *ctx, GLenum mode)
>  {
> -   switch (mode) {
> -   case GL_POINTS:
> -   case GL_LINES:
> -   case GL_LINE_LOOP:
> -   case GL_LINE_STRIP:
> -   case GL_TRIANGLES:
> -   case GL_TRIANGLE_STRIP:
> -   case GL_TRIANGLE_FAN:
> +   /* The overwhelmingly common case is (mode <= GL_TRIANGLE_FAN).  Test that
> +    * first and exit.  You would think that a switch-statement would be the
> +    * right approach, but at least GCC 4.7.2 generates some pretty dire code

Perhaps update this to 4.8.3 to match your commit message?

> +    * for the common case.
> +    */
> +   if (likely(mode <= GL_TRIANGLE_FAN))

I'm not sure if likely() is a good idea.  My understanding is the penalty for
getting it wrong is pretty hefty, and the other modes do happen.

Getting rid of the switch statement makes a lot of sense.  I like the new
approach.

With likely removed, this is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

>        return true;
> -   case GL_QUADS:
> -   case GL_QUAD_STRIP:
> -   case GL_POLYGON:
> +
> +   if (mode <= GL_POLYGON)
>        return (ctx->API == API_OPENGL_COMPAT);
> -   case GL_LINES_ADJACENCY:
> -   case GL_LINE_STRIP_ADJACENCY:
> -   case GL_TRIANGLES_ADJACENCY:
> -   case GL_TRIANGLE_STRIP_ADJACENCY:
> +
> +   if (mode <= GL_TRIANGLE_STRIP_ADJACENCY)
>        return _mesa_has_geometry_shaders(ctx);
> -   default:
> -      return false;
> -   }
> +
> +   return false;
>  }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141221/a73edcbf/attachment-0001.sig>