[Mesa-dev] [PATCH 1/2] glsl: add driconf to zero-init unintialized vars
Jason Ekstrand
jason at jlekstrand.net
Fri Jul 1 00:38:36 UTC 2016
On Thu, Jun 30, 2016 at 11:42 AM, Kenneth Graunke <kenneth at whitecape.org>
wrote:
> On Saturday, June 25, 2016 8:37:47 AM PDT Rob Clark wrote:
> > From: Rob Clark <robclark at freedesktop.org>
> >
> > Some games are sloppy.. perhaps because it is defined behavior for DX or
> > perhaps because nv blob driver defaults things to zero.
> >
> > So add driconf param to force uninitialized variables to default to zero.
> >
> > This issue was observed with rust, from steam store. But has surfaced
> > elsewhere in the past.
> >
> > Signed-off-by: Rob Clark <robclark at freedesktop.org>
> > ---
> > Note that I left out the drirc bit, since not entirely sure how to
> > identify this game. (I don't actually have the game, just working off
> > of an apitrace)
> >
> > Possibly worth mentioning that for the shaders using uninitialized vars
> > having zero-initializers lets constant-propagation get rid of a whole
> > lot of instructions. One shader I saw dropped to less than half of
> > it's original instruction count.
> >
> > Second patch in the series is just fixing an i965 bug that was exposed
> > by this patch.
>
> I'm a bit surprised to see this at the GLSL IR level...handling it for
> nir_ssa_undef would probably be simpler. But I suppose this works too.
>
That was my suggestion. I figured the gallium people would want it too.
>
> > src/compiler/glsl/ast_to_hir.cpp | 9 +++++++++
> > src/compiler/glsl/glsl_parser_extras.cpp | 1 +
> > src/compiler/glsl/glsl_parser_extras.h | 1 +
> > src/gallium/include/state_tracker/st_api.h | 1 +
> > src/gallium/state_trackers/dri/dri_screen.c | 2 ++
> > src/mesa/drivers/dri/common/xmlpool/t_options.h | 5 ++++-
> > src/mesa/drivers/dri/i965/brw_context.c | 2 ++
> > src/mesa/drivers/dri/i965/intel_screen.c | 4 ++++
> > src/mesa/main/mtypes.h | 5 +++++
> > src/mesa/state_tracker/st_extensions.c | 2 ++
> > 10 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> > index 0cfce68..d2c284f 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -4697,6 +4697,15 @@ ast_declarator_list::hir(exec_list *instructions,
> > apply_layout_qualifier_to_variable(&this->type->qualifier, var,
> state,
> > &loc);
> >
> > + if ((var->data.mode == ir_var_auto || var->data.mode ==
> ir_var_temporary)
> > + && (var->type->base_type >= GLSL_TYPE_UINT)
> > + && (var->type->base_type <= GLSL_TYPE_BOOL)
>
> I'd prefer:
>
> && (var->type->is_numeric() || var->type->is_boolean())
>
> Either way,
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> Patch 2 also gets an R-b with Jason's feedback (d[0] and u[0] on LHS)
> taken care of.
>
> > + && state->zero_init) {
> > + const ir_constant_data data = {0};
> > + var->data.has_initializer = true;
> > + var->constant_initializer = new(var) ir_constant(var->type,
> &data);
> > + }
> > +
> > if (this->type->qualifier.flags.q.invariant) {
> > if (!is_varying_var(var, state->stage)) {
> > _mesa_glsl_error(&loc, state,
> > diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> b/src/compiler/glsl/glsl_parser_extras.cpp
> > index 09f7477..fc2859a 100644
> > --- a/src/compiler/glsl/glsl_parser_extras.cpp
> > +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> > @@ -74,6 +74,7 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct
> gl_context *_ctx,
> > /* Set default language version and extensions */
> > this->language_version = 110;
> > this->forced_language_version = ctx->Const.ForceGLSLVersion;
> > + this->zero_init = ctx->Const.GLSLZeroInit;
> > this->es_shader = false;
> > this->ARB_texture_rectangle_enable = true;
> >
> > diff --git a/src/compiler/glsl/glsl_parser_extras.h
> b/src/compiler/glsl/glsl_parser_extras.h
> > index 8c43292..669b3d1 100644
> > --- a/src/compiler/glsl/glsl_parser_extras.h
> > +++ b/src/compiler/glsl/glsl_parser_extras.h
> > @@ -306,6 +306,7 @@ struct _mesa_glsl_parse_state {
> > bool es_shader;
> > unsigned language_version;
> > unsigned forced_language_version;
> > + bool zero_init;
> > gl_shader_stage stage;
> >
> > /**
> > diff --git a/src/gallium/include/state_tracker/st_api.h
> b/src/gallium/include/state_tracker/st_api.h
> > index 41daa47..21d5177 100644
> > --- a/src/gallium/include/state_tracker/st_api.h
> > +++ b/src/gallium/include/state_tracker/st_api.h
> > @@ -242,6 +242,7 @@ struct st_config_options
> > unsigned force_glsl_version;
> > boolean force_s3tc_enable;
> > boolean allow_glsl_extension_directive_midshader;
> > + boolean glsl_zero_init;
> > };
> >
> > /**
> > diff --git a/src/gallium/state_trackers/dri/dri_screen.c
> b/src/gallium/state_trackers/dri/dri_screen.c
> > index 2ac55c8..b16585a 100644
> > --- a/src/gallium/state_trackers/dri/dri_screen.c
> > +++ b/src/gallium/state_trackers/dri/dri_screen.c
> > @@ -74,6 +74,7 @@ const __DRIconfigOptionsExtension
> gallium_config_options = {
> >
> > DRI_CONF_SECTION_MISCELLANEOUS
> > DRI_CONF_ALWAYS_HAVE_DEPTH_BUFFER("false")
> > + DRI_CONF_GLSL_ZERO_INIT("false")
> > DRI_CONF_SECTION_END
> > DRI_CONF_END
> > };
> > @@ -98,6 +99,7 @@ dri_fill_st_options(struct st_config_options *options,
> > driQueryOptionb(optionCache, "force_s3tc_enable");
> > options->allow_glsl_extension_directive_midshader =
> > driQueryOptionb(optionCache,
> "allow_glsl_extension_directive_midshader");
> > + options->glsl_zero_init = driQueryOptionb(optionCache,
> "glsl_zero_init");
> > }
> >
> > static const __DRIconfig **
> > diff --git a/src/mesa/drivers/dri/common/xmlpool/t_options.h
> b/src/mesa/drivers/dri/common/xmlpool/t_options.h
> > index 4b298a4..341c376 100644
> > --- a/src/mesa/drivers/dri/common/xmlpool/t_options.h
> > +++ b/src/mesa/drivers/dri/common/xmlpool/t_options.h
> > @@ -337,7 +337,10 @@ DRI_CONF_OPT_BEGIN_B(always_have_depth_buffer, def)
> \
> > DRI_CONF_DESC(en,gettext("Create all visuals with a depth
> buffer")) \
> > DRI_CONF_OPT_END
> >
> > -
> > +#define DRI_CONF_GLSL_ZERO_INIT(def) \
> > +DRI_CONF_OPT_BEGIN_B(glsl_zero_init, def) \
> > + DRI_CONF_DESC(en,gettext("Force uninitialized variables to
> default to zero")) \
> > +DRI_CONF_OPT_END
> >
> > /**
> > * \brief Initialization configuration options
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> > index c7a66cb..66d4e94 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -826,6 +826,8 @@ brw_process_driconf_options(struct brw_context *brw)
> > ctx->Const.AllowGLSLExtensionDirectiveMidShader =
> > driQueryOptionb(options,
> "allow_glsl_extension_directive_midshader");
> >
> > + ctx->Const.GLSLZeroInit = driQueryOptionb(options, "glsl_zero_init");
> > +
> > brw->dual_color_blend_by_location =
> > driQueryOptionb(options, "dual_color_blend_by_location");
> > }
> > diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> > index 869119b..5aa28c6 100644
> > --- a/src/mesa/drivers/dri/i965/intel_screen.c
> > +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> > @@ -88,6 +88,10 @@ DRI_CONF_BEGIN
> > DRI_CONF_DESC(en, "Perform code generation at shader link time.")
> > DRI_CONF_OPT_END
> > DRI_CONF_SECTION_END
> > +
> > + DRI_CONF_SECTION_MISCELLANEOUS
> > + DRI_CONF_GLSL_ZERO_INIT("false")
> > + DRI_CONF_SECTION_END
> > DRI_CONF_END
> > };
> >
> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> > index 73ae55d..2210658 100644
> > --- a/src/mesa/main/mtypes.h
> > +++ b/src/mesa/main/mtypes.h
> > @@ -3514,6 +3514,11 @@ struct gl_constants
> > GLboolean AllowGLSLExtensionDirectiveMidShader;
> >
> > /**
> > + * Force uninitialized variables to default to zero.
> > + */
> > + GLboolean GLSLZeroInit;
> > +
> > + /**
> > * Does the driver support real 32-bit integers? (Otherwise,
> integers are
> > * simulated via floats.)
> > */
> > diff --git a/src/mesa/state_tracker/st_extensions.c
> b/src/mesa/state_tracker/st_extensions.c
> > index 412f598..e56d282 100644
> > --- a/src/mesa/state_tracker/st_extensions.c
> > +++ b/src/mesa/state_tracker/st_extensions.c
> > @@ -926,6 +926,8 @@ void st_init_extensions(struct pipe_screen *screen,
> > extensions->EXT_texture_integer = GL_FALSE;
> > }
> >
> > + consts->GLSLZeroInit = options->glsl_zero_init;
> > +
> > consts->UniformBooleanTrue = consts->NativeIntegers ? ~0U :
> fui(1.0f);
> >
> > /* Below are the cases which cannot be moved into tables easily. */
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160630/dda053f3/attachment-0001.html>
More information about the mesa-dev
mailing list