[Mesa-dev] [PATCH] nir: move pixel_center_integer/origin_upper_left to shader_info.fs
Jason Ekstrand
jason at jlekstrand.net
Fri Feb 8 17:55:45 UTC 2019
On Fri, Feb 8, 2019 at 8:33 AM Alejandro Piñeiro <apinheiro at igalia.com>
wrote:
> Although on GLSL those are set using a layout qualifier to
> gl_FragCoord builtin, they are basically a global mode. In fact, on
> SPIR-V they are set as an global ExecutionMode, not as a decoration
> for the builtin. With this change, we are just mapping them more
> similar to SPIR-V, instead of more similar to GLSL.
>
> FWIW, shader_info.fs already had pixel_center_integer, so this change
> also removes some redundancy.
>
> This change was needed because recently spirv_to_nir changed the order
> in which execution modes and variables are handled, so the variables
> didn't get the correct values. Now the info is set on the shader
> itself.
>
> Fixes: e68871f6a ("spirv: Handle constants and types before execution
> modes")
> ---
> src/compiler/glsl/glsl_to_nir.cpp | 9 +++++++--
> src/compiler/nir/nir.h | 8 --------
> src/compiler/nir/nir_lower_system_values.c | 6 ------
> src/compiler/nir/nir_lower_wpos_ytransform.c | 4 ++--
> src/compiler/shader_info.h | 6 ++++++
> src/compiler/spirv/spirv_to_nir.c | 4 ++--
> src/compiler/spirv/vtn_private.h | 2 --
> src/compiler/spirv/vtn_variables.c | 6 ------
> src/intel/blorp/blorp_blit.c | 2 +-
> src/intel/blorp/blorp_clear.c | 3 ++-
> src/intel/blorp/blorp_nir_builder.h | 1 -
> src/intel/vulkan/anv_nir_lower_input_attachments.c | 2 +-
> src/mesa/program/prog_to_nir.c | 8 ++++----
> 13 files changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/src/compiler/glsl/glsl_to_nir.cpp
> b/src/compiler/glsl/glsl_to_nir.cpp
> index 09599e4cee7..6ff20e8a692 100644
> --- a/src/compiler/glsl/glsl_to_nir.cpp
> +++ b/src/compiler/glsl/glsl_to_nir.cpp
> @@ -397,8 +397,13 @@ nir_visitor::visit(ir_variable *ir)
> }
>
> var->data.interpolation = ir->data.interpolation;
> - var->data.origin_upper_left = ir->data.origin_upper_left;
> - var->data.pixel_center_integer = ir->data.pixel_center_integer;
> + /* We only set the values of origin_upper_left and
> pixel_center_integer if
> + * they are set, to avoid following variables ovewritting them
> + */
> + if (ir->data.origin_upper_left)
> + shader->info.fs.origin_upper_left = ir->data.origin_upper_left;
> + if (ir->data.pixel_center_integer)
> + shader->info.fs.pixel_center_integer =
> ir->data.pixel_center_integer;
>
We should make this conditional on the variable being a fragment system
value and having a location of SYSTEM_VALUE_FRAG_COORD. That should also
prevent it from happening twice. Also, this could be made part of the info
gathering pass that gets run on GLSL shaders instead of part of glsl_to_nir.
Other than that, I really like this approach.
> var->data.location_frac = ir->data.location_frac;
>
> switch (ir->data.depth_layout) {
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index ff2c41faf27..bb2d3884acb 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -237,14 +237,6 @@ typedef struct nir_variable {
> */
> unsigned interpolation:2;
>
> - /**
> - * \name ARB_fragment_coord_conventions
> - * @{
> - */
> - unsigned origin_upper_left:1;
> - unsigned pixel_center_integer:1;
> - /*@}*/
> -
> /**
> * If non-zero, then this variable may be packed along with other
> variables
> * into a single varying slot, so this offset should be applied when
> diff --git a/src/compiler/nir/nir_lower_system_values.c
> b/src/compiler/nir/nir_lower_system_values.c
> index 7c1aa5fa801..68b0ea89c8d 100644
> --- a/src/compiler/nir/nir_lower_system_values.c
> +++ b/src/compiler/nir/nir_lower_system_values.c
> @@ -254,12 +254,6 @@ convert_block(nir_block *block, nir_builder *b)
> break;
> }
>
> - case SYSTEM_VALUE_FRAG_COORD:
> - assert(b->shader->info.stage == MESA_SHADER_FRAGMENT);
> - b->shader->info.fs.pixel_center_integer =
> - var->data.pixel_center_integer;
> - break;
> -
> default:
> break;
> }
> diff --git a/src/compiler/nir/nir_lower_wpos_ytransform.c
> b/src/compiler/nir/nir_lower_wpos_ytransform.c
> index 444e211b680..34a4801d66b 100644
> --- a/src/compiler/nir/nir_lower_wpos_ytransform.c
> +++ b/src/compiler/nir/nir_lower_wpos_ytransform.c
> @@ -181,7 +181,7 @@ lower_fragcoord(lower_wpos_ytransform_state *state,
> * u,h -> l,i: (99.5 + 0.5) * -1 + 100 = 0
> */
>
> - if (fragcoord->data.origin_upper_left) {
> + if (state->shader->info.fs.origin_upper_left) {
> /* Fragment shader wants origin in upper-left */
> if (options->fs_coord_origin_upper_left) {
> /* the driver supports upper-left origin */
> @@ -203,7 +203,7 @@ lower_fragcoord(lower_wpos_ytransform_state *state,
> }
> }
>
> - if (fragcoord->data.pixel_center_integer) {
> + if (state->shader->info.fs.pixel_center_integer) {
> /* Fragment shader wants pixel center integer */
> if (options->fs_coord_pixel_center_integer) {
> /* the driver supports pixel center integer */
> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
> index 3d871938751..12f869ebb52 100644
> --- a/src/compiler/shader_info.h
> +++ b/src/compiler/shader_info.h
> @@ -192,7 +192,13 @@ typedef struct shader_info {
>
> bool post_depth_coverage;
>
> + /**
> + * \name ARB_fragment_coord_conventions
> + * @{
> + */
> bool pixel_center_integer;
> + bool origin_upper_left:1;
> + /*@}*/
>
> bool pixel_interlock_ordered;
> bool pixel_interlock_unordered;
> diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> index 1cbc926c818..945214aca00 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -3784,7 +3784,7 @@ vtn_handle_execution_mode(struct vtn_builder *b,
> struct vtn_value *entry_point,
> switch(mode->exec_mode) {
> case SpvExecutionModeOriginUpperLeft:
> case SpvExecutionModeOriginLowerLeft:
> - b->origin_upper_left =
> + b->shader->info.fs.origin_upper_left =
> (mode->exec_mode == SpvExecutionModeOriginUpperLeft);
> break;
>
> @@ -3907,7 +3907,7 @@ vtn_handle_execution_mode(struct vtn_builder *b,
> struct vtn_value *entry_point,
> break;
>
> case SpvExecutionModePixelCenterInteger:
> - b->pixel_center_integer = true;
> + b->shader->info.fs.pixel_center_integer = true;
> break;
>
> case SpvExecutionModeXfb:
> diff --git a/src/compiler/spirv/vtn_private.h
> b/src/compiler/spirv/vtn_private.h
> index 63313034ba6..f3d54051885 100644
> --- a/src/compiler/spirv/vtn_private.h
> +++ b/src/compiler/spirv/vtn_private.h
> @@ -601,8 +601,6 @@ struct vtn_builder {
> const char *entry_point_name;
> struct vtn_value *entry_point;
> struct vtn_value *workgroup_size_builtin;
> - bool origin_upper_left;
> - bool pixel_center_integer;
> bool variable_pointers;
>
> struct vtn_function *func;
> diff --git a/src/compiler/spirv/vtn_variables.c
> b/src/compiler/spirv/vtn_variables.c
> index f6b458b7e78..51152520bb6 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1448,12 +1448,6 @@ apply_var_decoration(struct vtn_builder *b,
> case SpvBuiltInCullDistance:
> var_data->compact = true;
> break;
> - case SpvBuiltInFragCoord:
> - var_data->pixel_center_integer = b->pixel_center_integer;
> - /* fallthrough */
> - case SpvBuiltInSamplePosition:
> - var_data->origin_upper_left = b->origin_upper_left;
> - break;
> default:
> break;
> }
> diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c
> index b4bd9aac19a..ad93f58ae53 100644
> --- a/src/intel/blorp/blorp_blit.c
> +++ b/src/intel/blorp/blorp_blit.c
> @@ -87,7 +87,7 @@ brw_blorp_blit_vars_init(nir_builder *b, struct
> brw_blorp_blit_vars *v,
> v->frag_coord = nir_variable_create(b->shader, nir_var_shader_in,
> glsl_vec4_type(), "gl_FragCoord");
> v->frag_coord->data.location = VARYING_SLOT_POS;
> - v->frag_coord->data.origin_upper_left = true;
> + b->shader->info.fs.origin_upper_left = true;
>
> v->color_out = nir_variable_create(b->shader, nir_var_shader_out,
> glsl_vec4_type(), "gl_FragColor");
> diff --git a/src/intel/blorp/blorp_clear.c b/src/intel/blorp/blorp_clear.c
> index 0b26755e90a..181b492ab1f 100644
> --- a/src/intel/blorp/blorp_clear.c
> +++ b/src/intel/blorp/blorp_clear.c
> @@ -75,7 +75,7 @@ blorp_params_get_clear_kernel(struct blorp_batch *batch,
> nir_variable_create(b.shader, nir_var_shader_in,
> glsl_vec4_type(), "gl_FragCoord");
> frag_coord->data.location = VARYING_SLOT_POS;
> - frag_coord->data.origin_upper_left = true;
> + b.shader->info.fs.origin_upper_left = true;
>
> nir_ssa_def *pos = nir_f2i32(&b, nir_load_var(&b, frag_coord));
> nir_ssa_def *comp = nir_umod(&b, nir_channel(&b, pos, 0),
> @@ -969,6 +969,7 @@ blorp_params_get_mcs_partial_resolve_kernel(struct
> blorp_batch *batch,
> frag_color->data.location = FRAG_RESULT_COLOR;
>
> /* Do an MCS fetch and check if it is equal to the magic clear value */
> + b.shader->info.fs.origin_upper_left = true;
> nir_ssa_def *mcs =
> blorp_nir_txf_ms_mcs(&b, nir_f2i32(&b, blorp_nir_frag_coord(&b)),
> nir_load_layer_id(&b));
> diff --git a/src/intel/blorp/blorp_nir_builder.h
> b/src/intel/blorp/blorp_nir_builder.h
> index 7f23abdef4d..289cfb782c4 100644
> --- a/src/intel/blorp/blorp_nir_builder.h
> +++ b/src/intel/blorp/blorp_nir_builder.h
> @@ -31,7 +31,6 @@ blorp_nir_frag_coord(nir_builder *b)
> glsl_vec4_type(), "gl_FragCoord");
>
> frag_coord->data.location = VARYING_SLOT_POS;
> - frag_coord->data.origin_upper_left = true;
>
> return nir_load_var(b, frag_coord);
> }
> diff --git a/src/intel/vulkan/anv_nir_lower_input_attachments.c
> b/src/intel/vulkan/anv_nir_lower_input_attachments.c
> index 655e5844955..6568ec860fb 100644
> --- a/src/intel/vulkan/anv_nir_lower_input_attachments.c
> +++ b/src/intel/vulkan/anv_nir_lower_input_attachments.c
> @@ -35,7 +35,7 @@ load_frag_coord(nir_builder *b)
> nir_variable *pos = nir_variable_create(b->shader, nir_var_shader_in,
> glsl_vec4_type(), NULL);
> pos->data.location = VARYING_SLOT_POS;
> - pos->data.origin_upper_left = true;
> + b->shader->info.fs.origin_upper_left = true;
>
> return nir_load_var(b, pos);
> }
> diff --git a/src/mesa/program/prog_to_nir.c
> b/src/mesa/program/prog_to_nir.c
> index afa490cdb36..84ffdd0c510 100644
> --- a/src/mesa/program/prog_to_nir.c
> +++ b/src/mesa/program/prog_to_nir.c
> @@ -880,8 +880,8 @@ setup_registers_and_variables(struct ptn_compile *c)
>
> if (c->prog->Target == GL_FRAGMENT_PROGRAM_ARB) {
> if (i == VARYING_SLOT_POS) {
> - var->data.origin_upper_left = c->prog->OriginUpperLeft;
> - var->data.pixel_center_integer = c->prog->PixelCenterInteger;
> + shader->info.fs.origin_upper_left = c->prog->OriginUpperLeft;
> + shader->info.fs.pixel_center_integer =
> c->prog->PixelCenterInteger;
> } else if (i == VARYING_SLOT_FOGC) {
> /* fogcoord is defined as <f, 0.0, 0.0, 1.0>. Make the actual
> * input variable a float, and create a local containing the
> @@ -925,8 +925,8 @@ setup_registers_and_variables(struct ptn_compile *c)
>
> if (c->prog->Target == GL_FRAGMENT_PROGRAM_ARB &&
> i == SYSTEM_VALUE_FRAG_COORD) {
> - var->data.origin_upper_left = c->prog->OriginUpperLeft;
> - var->data.pixel_center_integer = c->prog->PixelCenterInteger;
> + shader->info.fs.origin_upper_left = c->prog->OriginUpperLeft;
> + shader->info.fs.pixel_center_integer =
> c->prog->PixelCenterInteger;
>
The fact that ARB programs also match kind-of confirms for me that this is
the right solution. That said, we probably shouldn't be handling this in
variable setup; we should probably, again, make it part of a more general
info-gathering.
--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190208/77a1204e/attachment-0001.html>
More information about the mesa-dev
mailing list