Hi Rob,
Not sure if there's a series that split things up ? I believe my
comments will be applicable either way.
On 26 March 2016 at 21:02, Rob Clark <robdcl...@gmail.com> wrote:
> From: Rob Clark <robcl...@freedesktop.org>
>
> Signed-off-by: Rob Clark <robcl...@freedesktop.org>
> ---
> src/compiler/nir/nir.h | 2 +
> .../drivers/freedreno/ir3/ir3_compiler_nir.c | 8 +
> src/mesa/Makefile.sources | 1 +
> src/mesa/state_tracker/st_glsl_to_nir.cpp | 425
> +++++++++++++++++++++
> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 40 +-
> src/mesa/state_tracker/st_glsl_to_tgsi.h | 5 +
> src/mesa/state_tracker/st_nir.h | 32 ++
> src/mesa/state_tracker/st_program.c | 148 ++++++-
> src/mesa/state_tracker/st_program.h | 6 +
> 9 files changed, 643 insertions(+), 24 deletions(-)
> create mode 100644 src/mesa/state_tracker/st_glsl_to_nir.cpp
>
> --- a/src/mesa/Makefile.sources
> +++ b/src/mesa/Makefile.sources
> @@ -487,6 +487,7 @@ STATETRACKER_FILES = \
> state_tracker/st_gen_mipmap.c \
> state_tracker/st_gen_mipmap.h \
> state_tracker/st_gl_api.h \
> + state_tracker/st_glsl_to_nir.cpp \
Add this to the new list (variable) as mentioned in earlier patch.
> --- /dev/null
> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
> +extern "C" {
> +
> +/* First half of converting glsl_to_nir.. this leaves things in a pre-
> + * nir_lower_io state, so that shader variants can more easily insert/
> + * replace variables, etc.
> + */
> +nir_shader *
> +st_glsl_to_nir(struct st_context *st, struct gl_program *prog,
> + struct gl_shader_program *shader_program,
> + gl_shader_stage stage)
Personally I'd annotate the 2-3 functions as extern "C" individually,
but that's just me ;-)
> +void
> +st_finalize_nir(struct st_context *st, struct gl_program *prog, nir_shader
> *nir)
> +struct gl_program *
> +st_nir_get_mesa_program(struct gl_context *ctx,
> + struct gl_shader_program *shader_program,
> + struct gl_shader *shader)
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index fa03c16..631ce3c 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -53,6 +53,7 @@
> #include "st_mesa_to_tgsi.h"
> #include "st_format.h"
> #include "st_glsl_types.h"
> +#include "st_nir.h"
>
>
> #define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
> @@ -6376,9 +6377,9 @@ out:
> * generating Mesa IR.
> */
> static struct gl_program *
> -get_mesa_program(struct gl_context *ctx,
> - struct gl_shader_program *shader_program,
> - struct gl_shader *shader)
> +get_mesa_program_tgsi(struct gl_context *ctx,
> + struct gl_shader_program *shader_program,
> + struct gl_shader *shader)
> {
> glsl_to_tgsi_visitor* v;
> struct gl_program *prog;
> @@ -6586,6 +6587,29 @@ get_mesa_program(struct gl_context *ctx,
> return prog;
> }
>
> +static struct gl_program *
> +get_mesa_program(struct gl_context *ctx,
> + struct gl_shader_program *shader_program,
> + struct gl_shader *shader)
> +{
> + struct pipe_screen *pscreen = ctx->st->pipe->screen;
> + unsigned ptarget = st_shader_stage_to_ptarget(shader->Stage);
> + enum pipe_shader_ir preferred_ir = (enum pipe_shader_ir)
> + pscreen->get_shader_param(pscreen, ptarget,
> PIPE_SHADER_CAP_PREFERRED_IR);
> + if (preferred_ir == PIPE_SHADER_IR_NIR) {
> + /* TODO only for GLSL VS/FS for now: */
> + switch (shader->Type) {
> + case GL_VERTEX_SHADER:
> + case GL_FRAGMENT_SHADER:
> + return st_nir_get_mesa_program(ctx, shader_program, shader);
> + default:
> + break;
> + }
> + }
> + return get_mesa_program_tgsi(ctx, shader_program, shader);
> +}
> +
> +
> extern "C" {
>
> static void
> @@ -6788,9 +6812,17 @@ st_translate_stream_output_info(glsl_to_tgsi_visitor
> *glsl_to_tgsi,
> const GLuint outputMapping[],
> struct pipe_stream_output_info *so)
> {
> - unsigned i;
> struct gl_transform_feedback_info *info =
> &glsl_to_tgsi->shader_program->LinkedTransformFeedback;
> + st_translate_stream_output_info2(info, outputMapping, so);
> +}
> +
> +void
> +st_translate_stream_output_info2(struct gl_transform_feedback_info *info,
> + const GLuint outputMapping[],
> + struct pipe_stream_output_info *so)
> +{
> + unsigned i;
>
> for (i = 0; i < info->NumOutputs; i++) {
> so->output[i].register_index =
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.h
> b/src/mesa/state_tracker/st_glsl_to_tgsi.h
> index 729295b..1986025 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.h
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.h
> @@ -63,6 +63,11 @@ st_translate_stream_output_info(struct
> glsl_to_tgsi_visitor *glsl_to_tgsi,
> const GLuint outputMapping[],
> struct pipe_stream_output_info *so);
>
> +void
> +st_translate_stream_output_info2(struct gl_transform_feedback_info *info,
> + const GLuint outputMapping[],
> + struct pipe_stream_output_info *so);
> +
> extern const unsigned _mesa_sysval_to_semantic[SYSTEM_VALUE_MAX];
>
> #ifdef __cplusplus
> diff --git a/src/mesa/state_tracker/st_nir.h b/src/mesa/state_tracker/st_nir.h
> index 1c07c4c..8c8a1d5 100644
> --- a/src/mesa/state_tracker/st_nir.h
> +++ b/src/mesa/state_tracker/st_nir.h
> @@ -23,6 +23,38 @@
>
> #pragma once
>
> +#include "st_context.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> typedef struct nir_shader nir_shader;
>
> void st_nir_lower_builtin(nir_shader *shader);
> +
> +#include "compiler/shader_enums.h"
Please move any header inclusions before the extern C guard/wrapper.
> +
> +nir_shader * st_glsl_to_nir(struct st_context *st, struct gl_program *prog,
> + struct gl_shader_program *shader_program,
> + gl_shader_stage stage);
> +
> +void st_finalize_nir(struct st_context *st, struct gl_program *prog,
> nir_shader *nir);
> +
> +struct gl_program *
> +st_nir_get_mesa_program(struct gl_context *ctx,
> + struct gl_shader_program *shader_program,
> + struct gl_shader *shader);
> +
> +
> +#define OPT(nir, pass, ...) ({ \
> + bool this_progress = false; \
> + NIR_PASS(this_progress, nir, pass, ##__VA_ARGS__); \
> + this_progress; \
> +})
Not 100% sure but I believe the above usage to be GCC extension.
Although we nuked non gcc/clang, I'm not sure how MSVC will cope (will
it even bother if we're not building any code that uses the macro).
From memory there were a handful of similar hunks elsewhere in nir
(related) code.
> +
> +#define OPT_V(nir, pass, ...) NIR_PASS_V(nir, pass, ##__VA_ARGS__)
> +
Please add namespace for the OPT macros - ST_NIR_OPT, just NIR_OPT or otherwise.
> +#ifdef __cplusplus
> +}
> +#endif
> diff --git a/src/mesa/state_tracker/st_program.c
> b/src/mesa/state_tracker/st_program.c
> index 80dcfd8..048fe4b 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -38,6 +38,8 @@
> #include "program/prog_print.h"
> #include "program/programopt.h"
>
> +#include "compiler/nir/nir.h"
> +
> #include "pipe/p_context.h"
> #include "pipe/p_defines.h"
> #include "pipe/p_shader_tokens.h"
> @@ -53,6 +55,7 @@
> #include "st_context.h"
> #include "st_program.h"
> #include "st_mesa_to_tgsi.h"
> +#include "st_nir.h"
> #include "cso_cache/cso_context.h"
>
>
> @@ -69,10 +72,10 @@ delete_vp_variant(struct st_context *st, struct
> st_vp_variant *vpv)
>
> if (vpv->draw_shader)
> draw_delete_vertex_shader( st->draw, vpv->draw_shader );
> -
> - if (vpv->tgsi.tokens)
> +
> + if (((vpv->tgsi.type == PIPE_SHADER_IR_TGSI)) && vpv->tgsi.tokens)
> ureg_free_tokens(vpv->tgsi.tokens);
> -
> +
> free( vpv );
> }
>
> @@ -95,7 +98,7 @@ st_release_vp_variants( struct st_context *st,
>
> stvp->variants = NULL;
>
> - if (stvp->tgsi.tokens) {
> + if ((stvp->tgsi.type == PIPE_SHADER_IR_TGSI) && stvp->tgsi.tokens) {
> tgsi_free_tokens(stvp->tgsi.tokens);
> stvp->tgsi.tokens = NULL;
> }
> @@ -132,7 +135,7 @@ st_release_fp_variants(struct st_context *st, struct
> st_fragment_program *stfp)
>
> stfp->variants = NULL;
>
> - if (stfp->tgsi.tokens) {
> + if ((stfp->tgsi.type == PIPE_SHADER_IR_TGSI) && stfp->tgsi.tokens) {
> ureg_free_tokens(stfp->tgsi.tokens);
> stfp->tgsi.tokens = NULL;
> }
> @@ -355,9 +358,23 @@ st_translate_vertex_program(struct st_context *st,
> output_semantic_name[num_outputs] = TGSI_SEMANTIC_EDGEFLAG;
> output_semantic_index[num_outputs] = 0;
>
> - if (!stvp->glsl_to_tgsi)
> + if (!stvp->glsl_to_tgsi && !stvp->shader_program)
> _mesa_remove_output_reads(&stvp->Base.Base, PROGRAM_OUTPUT);
>
> + if (stvp->shader_program) {
> + nir_shader *nir = st_glsl_to_nir(st, &stvp->Base.Base,
> + stvp->shader_program,
> + MESA_SHADER_VERTEX);
> +
> + stvp->tgsi.type = PIPE_SHADER_IR_NIR;
> + stvp->tgsi.ir.nir = nir;
> +
> +
> st_translate_stream_output_info2(&stvp->shader_program->LinkedTransformFeedback,
> + stvp->result_to_output,
> + &stvp->tgsi.stream_output);
> + return true;
> + }
> +
In general you one might get away with adding a few stubs for
st_glsl_to_nir/nir_shader_clone when building without NIR, to avoid
polluting the code with #ifdef. Not too sure though...
> + if (stvp->tgsi.type == PIPE_SHADER_IR_NIR) {
> + vpv->tgsi.type = PIPE_SHADER_IR_NIR;
> + vpv->tgsi.ir.nir = nir_shader_clone(NULL, stvp->tgsi.ir.nir);
> + if (key->clamp_color)
> + OPT_V(vpv->tgsi.ir.nir, nir_lower_clamp_color_outputs);
> + if (key->passthrough_edgeflags)
> + OPT_V(vpv->tgsi.ir.nir, nir_lower_passthrough_edgeflags);
> +
> + st_finalize_nir(st, &stvp->Base.Base, vpv->tgsi.ir.nir);
> +
> + vpv->driver_shader = pipe->create_vs_state(pipe, &vpv->tgsi);
> + /* driver takes ownership of IR: */
> + vpv->tgsi.ir.nir = NULL;
> + return vpv;
> + }
> +
Esp. for hunks like this one.
-Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
