<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Feb 8, 2019 at 8:33 AM Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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; </blockquote><div> </div><div>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.</div><div> </div><div>Other than that, I really like this approach. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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; </blockquote><div> </div><div>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.</div><div> </div><div>--Jason </div></div></div>