Re: [Mesa-dev] [PATCH] mesa/glsl: stop using GL shader type internally

Thu, 16 Jun 2016 07:20:07 -0700 

On 05/06/16 04:17, Timothy Arceri wrote:

Instead use the internal gl_shader_stage enum everywhere. This
makes things more consistent and gets rid of unnecessary
conversions.

Ideally it would be nice to remove the Type field from gl_shader
altogether but currently it is used to differentiate between
gl_shader and gl_shader_program in the ShaderObjects hash table.
---
  src/compiler/glsl/builtin_functions.cpp      |  2 +-
  src/compiler/glsl/link_varyings.cpp          |  2 +-
  src/compiler/glsl/linker.cpp                 |  2 +-
  src/compiler/glsl/standalone.cpp             |  2 +-
  src/compiler/glsl/standalone_scaffolding.cpp |  7 +++----
  src/compiler/glsl/standalone_scaffolding.h   |  2 +-
  src/mesa/drivers/common/meta.c               | 12 +++++------
  src/mesa/drivers/common/meta.h               |  5 -----
  src/mesa/drivers/dri/i965/brw_link.cpp       |  5 ++---
  src/mesa/drivers/dri/i965/brw_shader.h       |  3 ++-
  src/mesa/main/dd.h                           |  2 +-
  src/mesa/main/ff_fragment_shader.cpp         |  2 +-
  src/mesa/main/shaderapi.c                    | 20 ++++++++++---------
  src/mesa/main/shaderobj.c                    |  6 ++----
  src/mesa/state_tracker/st_glsl_to_nir.cpp    |  6 +++---
  src/mesa/state_tracker/st_glsl_to_tgsi.cpp   | 30 ++++++++++++++--------------
  16 files changed, 51 insertions(+), 57 deletions(-)

diff --git a/src/compiler/glsl/builtin_functions.cpp 
b/src/compiler/glsl/builtin_functions.cpp
index edd02bb..87d22e6 100644
--- a/src/compiler/glsl/builtin_functions.cpp
+++ b/src/compiler/glsl/builtin_functions.cpp
@@ -948,7 +948,7 @@ builtin_builder::create_shader()
      * GLSL utility code that could be linked against any stage, so just
      * arbitrarily pick GL_VERTEX_SHADER.
      */
-   shader = _mesa_new_shader(NULL, 0, GL_VERTEX_SHADER);
+   shader = _mesa_new_shader(NULL, 0, MESA_SHADER_VERTEX);
     shader->symbols = new(mem_ctx) glsl_symbol_table;

     gl_ModelViewProjectionMatrix =
diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index a286e77..2ff7d80 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -2111,7 +2111,7 @@ assign_varying_locations(struct gl_context *ctx,
            * within a patch and can be used as shared memory.
            */
           if (input_var || (prog->SeparateShader && consumer == NULL) ||
-             producer->Type == GL_TESS_CONTROL_SHADER) {
+             producer->Stage == MESA_SHADER_TESS_CTRL) {
              matches.record(output_var, input_var);
           }

diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 43719f4..e0e09e9 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -2237,7 +2237,7 @@ link_intrastage_shaders(void *mem_ctx,
        return NULL;
     }

-   gl_shader *linked = ctx->Driver.NewShader(NULL, 0, main->Type);
+   gl_shader *linked = ctx->Driver.NewShader(NULL, 0, shader_list[0]->Stage);
     linked->ir = new(linked) exec_list;
     clone_ir_list(mem_ctx, linked->ir, main->ir);

diff --git a/src/compiler/glsl/standalone.cpp b/src/compiler/glsl/standalone.cpp
index c3afbc3..9b77b3a 100644
--- a/src/compiler/glsl/standalone.cpp
+++ b/src/compiler/glsl/standalone.cpp
@@ -419,7 +419,7 @@ standalone_compile_shader(const struct standalone_options 
*_options,
              continue;

           shader->Program = rzalloc(shader, gl_program);
-         init_gl_program(shader->Program, shader->Type);
+         init_gl_program(shader->Program, shader->Stage);
        }
     }

diff --git a/src/compiler/glsl/standalone_scaffolding.cpp 
b/src/compiler/glsl/standalone_scaffolding.cpp
index e7f6555..94938ef 100644
--- a/src/compiler/glsl/standalone_scaffolding.cpp
+++ b/src/compiler/glsl/standalone_scaffolding.cpp
@@ -76,17 +76,16 @@ _mesa_shader_debug(struct gl_context *, GLenum, GLuint *,
  }

  struct gl_shader *
-_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type)
+_mesa_new_shader(struct gl_context *ctx, GLuint name, gl_shader_stage stage)
  {
     struct gl_shader *shader;

     (void) ctx;

-   assert(type == GL_FRAGMENT_SHADER || type == GL_VERTEX_SHADER);
+   assert(stage == MESA_SHADER_FRAGMENT || stage == MESA_SHADER_VERTEX);
     shader = rzalloc(NULL, struct gl_shader);
     if (shader) {
-      shader->Type = type;
-      shader->Stage = _mesa_shader_enum_to_shader_stage(type);
+      shader->Stage = stage;
        shader->Name = name;
        shader->RefCount = 1;
     }
diff --git a/src/compiler/glsl/standalone_scaffolding.h 
b/src/compiler/glsl/standalone_scaffolding.h
index 6cef784..cfac883 100644
--- a/src/compiler/glsl/standalone_scaffolding.h
+++ b/src/compiler/glsl/standalone_scaffolding.h
@@ -47,7 +47,7 @@ _mesa_reference_program_(struct gl_context *ctx, struct 
gl_program **ptr,
                         struct gl_shader *sh);

  extern "C" struct gl_shader *
-_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type);
+_mesa_new_shader(struct gl_context *ctx, GLuint name, gl_shader_stage stage);

  extern "C" void
  _mesa_delete_shader(struct gl_context *ctx, struct gl_shader *sh);
diff --git a/src/mesa/drivers/common/meta.c b/src/mesa/drivers/common/meta.c
index 6dcbc8b..3c86305 100644
--- a/src/mesa/drivers/common/meta.c
+++ b/src/mesa/drivers/common/meta.c
@@ -121,14 +121,14 @@ _mesa_meta_framebuffer_texture_image(struct gl_context 
*ctx,
                               level, layer, false, __func__);
  }

-struct gl_shader *
-_mesa_meta_compile_shader_with_debug(struct gl_context *ctx, GLenum target,
-                                     const GLcharARB *source)
+static struct gl_shader *
+meta_compile_shader_with_debug(struct gl_context *ctx, gl_shader_stage stage,
+                               const GLcharARB *source)
  {
     const GLuint name = ~0;
     struct gl_shader *sh;

-   sh = ctx->Driver.NewShader(ctx, name, target);
+   sh = ctx->Driver.NewShader(ctx, name, stage);
     sh->Source = strdup(source);
     sh->CompileStatus = false;
     _mesa_compile_shader(ctx, sh);
@@ -183,9 +183,9 @@ _mesa_meta_compile_and_link_program(struct gl_context *ctx,
     sh_prog->NumShaders = 2;
     sh_prog->Shaders = malloc(2 * sizeof(struct gl_shader *));
     sh_prog->Shaders[0] =
-      _mesa_meta_compile_shader_with_debug(ctx, GL_VERTEX_SHADER, vs_source);
+      meta_compile_shader_with_debug(ctx, MESA_SHADER_VERTEX, vs_source);
     sh_prog->Shaders[1] =
-      _mesa_meta_compile_shader_with_debug(ctx, GL_FRAGMENT_SHADER, fs_source);
+      meta_compile_shader_with_debug(ctx, MESA_SHADER_FRAGMENT, fs_source);

     _mesa_meta_link_program_with_debug(ctx, sh_prog);

diff --git a/src/mesa/drivers/common/meta.h b/src/mesa/drivers/common/meta.h
index 0a7321c..ba83a6d 100644
--- a/src/mesa/drivers/common/meta.h
+++ b/src/mesa/drivers/common/meta.h
@@ -577,11 +577,6 @@ _mesa_meta_DrawTex(struct gl_context *ctx, GLfloat x, 
GLfloat y, GLfloat z,
  void
  _mesa_meta_drawbuffers_from_bitfield(GLbitfield bits);

-struct gl_shader *
-_mesa_meta_compile_shader_with_debug(struct gl_context *ctx, GLenum target,
-                                     const GLcharARB *source);
-
-
  void
  _mesa_meta_link_program_with_debug(struct gl_context *ctx,
                                     struct gl_shader_program *sh_prog);
diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp 
b/src/mesa/drivers/dri/i965/brw_link.cpp
index b340da3..c8bea97 100644
--- a/src/mesa/drivers/dri/i965/brw_link.cpp
+++ b/src/mesa/drivers/dri/i965/brw_link.cpp
@@ -189,14 +189,13 @@ process_glsl_ir(gl_shader_stage stage,
  }

  extern "C" struct gl_shader *
-brw_new_shader(struct gl_context *ctx, GLuint name, GLuint type)
+brw_new_shader(struct gl_context *ctx, GLuint name, gl_shader_stage stage)
  {
     struct brw_shader *shader;

     shader = rzalloc(NULL, struct brw_shader);
     if (shader) {
-      shader->base.Type = type;
-      shader->base.Stage = _mesa_shader_enum_to_shader_stage(type);
+      shader->base.Stage = stage;
        shader->base.Name = name;
        _mesa_init_shader(ctx, &shader->base);
     }
diff --git a/src/mesa/drivers/dri/i965/brw_shader.h 
b/src/mesa/drivers/dri/i965/brw_shader.h
index 4da1364..4096791 100644
--- a/src/mesa/drivers/dri/i965/brw_shader.h
+++ b/src/mesa/drivers/dri/i965/brw_shader.h
@@ -291,7 +291,8 @@ bool brw_cs_precompile(struct gl_context *ctx,
                         struct gl_program *prog);

  GLboolean brw_link_shader(struct gl_context *ctx, struct gl_shader_program 
*prog);
-struct gl_shader *brw_new_shader(struct gl_context *ctx, GLuint name, GLuint 
type);
+struct gl_shader *brw_new_shader(struct gl_context *ctx, GLuint name,
+                                 gl_shader_stage stage);

  int type_size_scalar(const struct glsl_type *type);
  int type_size_vec4(const struct glsl_type *type);
diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
index 5710a34..bf0c6e1 100644
--- a/src/mesa/main/dd.h
+++ b/src/mesa/main/dd.h
@@ -785,7 +785,7 @@ struct dd_function_table {
      */
     /*@{*/
     struct gl_shader *(*NewShader)(struct gl_context *ctx,
-                                  GLuint name, GLenum type);
+                                  GLuint name, gl_shader_stage stage);
     void (*UseProgram)(struct gl_context *ctx, struct gl_shader_program 
*shProg);
     /*@}*/

diff --git a/src/mesa/main/ff_fragment_shader.cpp 
b/src/mesa/main/ff_fragment_shader.cpp
index 4967eb3..d1c7444 100644
--- a/src/mesa/main/ff_fragment_shader.cpp
+++ b/src/mesa/main/ff_fragment_shader.cpp
@@ -1199,7 +1199,7 @@ create_new_program(struct gl_context *ctx, struct 
state_key *key)
     _mesa_glsl_parse_state *state;

     p.mem_ctx = ralloc_context(NULL);
-   p.shader = ctx->Driver.NewShader(ctx, 0, GL_FRAGMENT_SHADER);
+   p.shader = ctx->Driver.NewShader(ctx, 0, MESA_SHADER_FRAGMENT);
     p.shader->ir = new(p.shader) exec_list;
     state = new(p.shader) _mesa_glsl_parse_state(ctx, MESA_SHADER_FRAGMENT,
                                                p.shader);
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 9351976..51f46b9 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -264,7 +264,7 @@ attach_shader(struct gl_context *ctx, GLuint program, 
GLuint shader)
           _mesa_error(ctx, GL_INVALID_OPERATION, "glAttachShader");
           return;
        } else if (same_type_disallowed &&
-                 shProg->Shaders[i]->Type == sh->Type) {
+                 shProg->Shaders[i]->Stage == sh->Stage) {
          /* Shader with the same type is already attached to this program,
           * OpenGL ES 2.0 and 3.0 specs say:
           *
@@ -307,7 +307,9 @@ create_shader(struct gl_context *ctx, GLenum type)

     _mesa_HashLockMutex(ctx->Shared->ShaderObjects);
     name = _mesa_HashFindFreeKeyBlock(ctx->Shared->ShaderObjects, 1);
-   sh = ctx->Driver.NewShader(ctx, name, type);
+   sh = ctx->Driver.NewShader(ctx, name,
+                              _mesa_shader_enum_to_shader_stage(type));
+   sh->Type = type;
     _mesa_HashInsertLocked(ctx->Shared->ShaderObjects, name, sh);
     _mesa_HashUnlockMutex(ctx->Shared->ShaderObjects);

@@ -429,11 +431,11 @@ detach_shader(struct gl_context *ctx, GLuint program, 
GLuint shader)
  #ifdef DEBUG
           /* sanity check - make sure the new list's entries are sensible */
           for (j = 0; j < shProg->NumShaders; j++) {
-            assert(shProg->Shaders[j]->Type == GL_VERTEX_SHADER ||
-                   shProg->Shaders[j]->Type == GL_TESS_CONTROL_SHADER ||
-                   shProg->Shaders[j]->Type == GL_TESS_EVALUATION_SHADER ||
-                   shProg->Shaders[j]->Type == GL_GEOMETRY_SHADER ||
-                   shProg->Shaders[j]->Type == GL_FRAGMENT_SHADER);
+            assert(shProg->Shaders[j]->Stage == MESA_SHADER_VERTEX ||
+                   shProg->Shaders[j]->Stage == MESA_SHADER_TESS_CTRL ||
+                   shProg->Shaders[j]->Stage == MESA_SHADER_TESS_EVAL ||
+                   shProg->Shaders[j]->Stage == MESA_SHADER_GEOMETRY ||
+                   shProg->Shaders[j]->Stage == MESA_SHADER_FRAGMENT);
              assert(shProg->Shaders[j]->RefCount > 0);
           }
  #endif
@@ -1065,9 +1067,9 @@ _mesa_link_program(struct gl_context *ctx, struct 
gl_shader_program *shProg)
                     shProg->LinkStatus ? "Success" : "Failed");

        for (i = 0; i < shProg->NumShaders; i++) {
-         printf(" shader %u, type 0x%x\n",
+         printf(" shader %u, stage %u\n",
                        shProg->Shaders[i]->Name,
-                      shProg->Shaders[i]->Type);
+                      shProg->Shaders[i]->Stage);
        }
     }
  }
diff --git a/src/mesa/main/shaderobj.c b/src/mesa/main/shaderobj.c
index fc1e3e3..030c84f 100644
--- a/src/mesa/main/shaderobj.c
+++ b/src/mesa/main/shaderobj.c
@@ -99,14 +99,12 @@ _mesa_init_shader(struct gl_context *ctx, struct gl_shader 
*shader)
   * Called via ctx->Driver.NewShader()
   */
  struct gl_shader *
-_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type)
+_mesa_new_shader(struct gl_context *ctx, GLuint name, gl_shader_stage stage)
The implementation in shaderobj.c was changed, but the declaration in shaderobj.h was not. And as result Clang on Windows is now failing with: 

In file included from src\mesa\main\shaderobj.c:38:
src\mesa\main/shaderobj.h(132,1) : warning: control may reach end of non-void function [-Wreturn-type] 
}
^
src\mesa\main/shaderobj.h(175,1) : warning: control may reach end of non-void function [-Wreturn-type] 
}
^
src\mesa\main/shaderobj.h(195,1) : warning: control may reach end of non-void function [-Wreturn-type] 
}
^
src\mesa\main\shaderobj.c(102,1) : error: conflicting types for '_mesa_new_shader' 
_mesa_new_shader(struct gl_context *ctx, GLuint name, gl_shader_stage stage)
^
src\mesa\main/shaderobj.h(83,1) :  note: previous declaration is here
_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type);
^
src\mesa\main\shaderobj.c(439,22) : warning: incompatible pointer types assigning to 'struct gl_shader *(*)(struct gl_context *, GLuint, gl_shader_stage)' (aka 'struct gl_shader *(*)(struct gl_context *, unsigned int, gl_shader_stage)') from 'struct gl_shader *(struct gl_context *, GLuint, GLenum)' (aka 'struct gl_shader *(struct gl_context *, unsigned int, unsigned int)') [-Wincompatible-pointer-types] 
   driver->NewShader = _mesa_new_shader;
                     ^ ~~~~~~~~~~~~~~~~
4 warnings and 1 error generated.
scons: *** [build\windows-x86-debug\mesa\main\shaderobj.obj] Error 1
I'm surprised GCC/MSVC doesn't warn about this. "GLenum" and "gl_shader_stage" are completely different things.
Even weirder: Clang on Linux doesn't warn about it neither. I wish there was some way to tell Clang/GCC to warn about these things. It seems useful. 


Jose
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to