From: Alejandro Piñeiro Date: Thu, 7 Feb 2019 17:43:58 +0000 (+0100) Subject: nir, glsl: move pixel_center_integer/origin_upper_left to shader_info.fs X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=0629b2a462a1dfc729fb487419b3c2749ef9e728;p=mesa.git nir, glsl: move pixel_center_integer/origin_upper_left to shader_info.fs On GLSL that info is set as a layout qualifier when redeclaring gl_FragCoord, so somehow tied to a specific variable. But in practice, they behave as a global of the shader. On ARB programs they are set using a global OPTION (defined at ARB_fragment_coord_conventions), and on SPIR-V using ExecutionModes, that are also not tied specifically to the builtin. This patch moves that info from nir variable and ir variable to nir shader and gl_program shader_info respectively, so the map is more similar to SPIR-V, and ARB programs, instead of more similar to GLSL. FWIW, shader_info.fs already had pixel_center_integer, so this change also removes some redundancy. Also, as struct gl_program also includes a shader_info, we removed gl_program::OriginUpperLeft and PixelCenterInteger, as it would be superfluous. 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, and we don't need to go back to the builtin variable to set it. Fixes: e68871f6a ("spirv: Handle constants and types before execution modes") v2: (Jason) * glsl_to_nir: get the info before glsl_to_nir, while all the rest of the info gathering is happening * prog_to_nir: gather the info on a general info-gathering pass, not on variable setup. v3: (Jason) * Squash with the patch that removes that info from ir variable * anv: assert that OriginUpperLeft is true. It should be already set by spirv_to_nir. * blorp: set origin_upper_left on its core "compile fragment shader", not just on some specific places (for this we added an helper on a previous patch). * prog_to_nir: no need to gather specifically this fragcoord modes as the full gl_program shader_info is copied. * spirv_to_nir: assert that we are a fragment shader when handling this execution modes. v4: (reported by failing gitlab pipeline #18750) * state_tracker: update too due changes on ir.h/gl_program v5: * blorp: minor change after change on previous patch * radeonsi: update due this change. v6: (Timothy Arceri) * prog_to_nir: remove extra whitespace * shader_info: don't use :1 on origin_upper_left * glsl: program.fs.origin_upper_left/pixel_center_integer can be move out of the shader list loop --- diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp index 620153e6a34..f68ed46435b 100644 --- a/src/compiler/glsl/ast_to_hir.cpp +++ b/src/compiler/glsl/ast_to_hir.cpp @@ -3670,8 +3670,6 @@ apply_layout_qualifier_to_variable(const struct ast_type_qualifier *qual, state->fs_redeclares_gl_fragcoord_with_no_layout_qualifiers; } - var->data.pixel_center_integer = qual->flags.q.pixel_center_integer; - var->data.origin_upper_left = qual->flags.q.origin_upper_left; if ((qual->flags.q.origin_upper_left || qual->flags.q.pixel_center_integer) && (strcmp(var->name, "gl_FragCoord") != 0)) { const char *const qual_string = (qual->flags.q.origin_upper_left) @@ -4290,10 +4288,13 @@ get_variable_being_redeclared(ir_variable **var_ptr, YYLTYPE loc, && strcmp(var->name, "gl_FragCoord") == 0) { /* Allow redeclaration of gl_FragCoord for ARB_fcc layout * qualifiers. + * + * We don't really need to do anything here, just allow the + * redeclaration. Any error on the gl_FragCoord is handled on the ast + * level at apply_layout_qualifier_to_variable using the + * ast_type_qualifier and _mesa_glsl_parse_state, or later at + * linker.cpp. */ - earlier->data.origin_upper_left = var->data.origin_upper_left; - earlier->data.pixel_center_integer = var->data.pixel_center_integer; - /* According to section 4.3.7 of the GLSL 1.30 spec, * the following built-in varaibles can be redeclared with an * interpolation qualifier: diff --git a/src/compiler/glsl/glsl_to_nir.cpp b/src/compiler/glsl/glsl_to_nir.cpp index d62de862fac..09a4f19f6f2 100644 --- a/src/compiler/glsl/glsl_to_nir.cpp +++ b/src/compiler/glsl/glsl_to_nir.cpp @@ -168,6 +168,11 @@ glsl_to_nir(const struct gl_shader_program *shader_prog, shader->info.has_transform_feedback_varyings |= shader_prog->last_vert_prog->sh.LinkedTransformFeedback->NumVarying > 0; + if (shader->info.stage == MESA_SHADER_FRAGMENT) { + shader->info.fs.pixel_center_integer = sh->Program->info.fs.pixel_center_integer; + shader->info.fs.origin_upper_left = sh->Program->info.fs.origin_upper_left; + } + return shader; } @@ -398,8 +403,6 @@ 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; var->data.location_frac = ir->data.location_frac; switch (ir->data.depth_layout) { diff --git a/src/compiler/glsl/ir.cpp b/src/compiler/glsl/ir.cpp index 1d1a56ae9a5..77e37161b74 100644 --- a/src/compiler/glsl/ir.cpp +++ b/src/compiler/glsl/ir.cpp @@ -1725,8 +1725,6 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name, this->data.warn_extension_index = 0; this->constant_value = NULL; this->constant_initializer = NULL; - this->data.origin_upper_left = false; - this->data.pixel_center_integer = false; this->data.depth_layout = ir_depth_layout_none; this->data.used = false; this->data.always_active_io = false; diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h index d05d1998a50..fa124863d09 100644 --- a/src/compiler/glsl/ir.h +++ b/src/compiler/glsl/ir.h @@ -711,14 +711,6 @@ public: */ unsigned interpolation:2; - /** - * \name ARB_fragment_coord_conventions - * @{ - */ - unsigned origin_upper_left:1; - unsigned pixel_center_integer:1; - /*@}*/ - /** * Was the location explicitly set in the shader? * diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index 2d76e852f47..5a950950b7c 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -2049,9 +2049,11 @@ link_fs_inout_layout_qualifiers(struct gl_shader_program *prog, shader->SampleInterlockOrdered; linked_shader->Program->info.fs.sample_interlock_unordered |= shader->SampleInterlockUnordered; - linked_shader->Program->sh.fs.BlendSupport |= shader->BlendSupport; } + + linked_shader->Program->info.fs.pixel_center_integer = pixel_center_integer; + linked_shader->Program->info.fs.origin_upper_left = origin_upper_left; } /** diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 94ca6c46468..190b7af7c28 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 ea6f9a16375..8fbdfab1a8e 100644 --- a/src/compiler/shader_info.h +++ b/src/compiler/shader_info.h @@ -195,7 +195,13 @@ typedef struct shader_info { bool post_depth_coverage; + /** + * \name ARB_fragment_coord_conventions + * @{ + */ bool pixel_center_integer; + bool origin_upper_left; + /*@}*/ 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 8c07542f832..f5511587be8 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -3785,7 +3785,8 @@ 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 = + vtn_assert(b->shader->info.stage == MESA_SHADER_FRAGMENT); + b->shader->info.fs.origin_upper_left = (mode->exec_mode == SpvExecutionModeOriginUpperLeft); break; @@ -3908,7 +3909,8 @@ vtn_handle_execution_mode(struct vtn_builder *b, struct vtn_value *entry_point, break; case SpvExecutionModePixelCenterInteger: - b->pixel_center_integer = true; + vtn_assert(b->shader->info.stage == MESA_SHADER_FRAGMENT); + 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/gallium/drivers/radeonsi/si_shader_nir.c b/src/gallium/drivers/radeonsi/si_shader_nir.c index f401221526d..812c2172366 100644 --- a/src/gallium/drivers/radeonsi/si_shader_nir.c +++ b/src/gallium/drivers/radeonsi/si_shader_nir.c @@ -440,7 +440,7 @@ void si_nir_scan_shader(const struct nir_shader *nir, /* Fragment shader position is a system value. */ if (nir->info.stage == MESA_SHADER_FRAGMENT && variable->data.location == VARYING_SLOT_POS) { - if (variable->data.pixel_center_integer) + if (nir->info.fs.pixel_center_integer) info->properties[TGSI_PROPERTY_FS_COORD_PIXEL_CENTER] = TGSI_FS_COORD_PIXEL_CENTER_INTEGER; diff --git a/src/intel/blorp/blorp_blit.c b/src/intel/blorp/blorp_blit.c index 9bea4ffbe2c..f879ec4141f 100644 --- a/src/intel/blorp/blorp_blit.c +++ b/src/intel/blorp/blorp_blit.c @@ -87,7 +87,6 @@ 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; 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 1ca0c44835e..657f5970d48 100644 --- a/src/intel/blorp/blorp_clear.c +++ b/src/intel/blorp/blorp_clear.c @@ -74,7 +74,6 @@ 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; 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), diff --git a/src/intel/blorp/blorp_nir_builder.h b/src/intel/blorp/blorp_nir_builder.h index 28c34d19da6..9664bdbcd27 100644 --- a/src/intel/blorp/blorp_nir_builder.h +++ b/src/intel/blorp/blorp_nir_builder.h @@ -32,6 +32,8 @@ blorp_nir_init_shader(nir_builder *b, nir_builder_init_simple_shader(b, mem_ctx, stage, NULL); if (name != NULL) b->shader->info.name = ralloc_strdup(b->shader, name); + if (stage == MESA_SHADER_FRAGMENT) + b->shader->info.fs.origin_upper_left = true; } static inline nir_ssa_def * @@ -42,7 +44,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..9c6f9f434d7 100644 --- a/src/intel/vulkan/anv_nir_lower_input_attachments.c +++ b/src/intel/vulkan/anv_nir_lower_input_attachments.c @@ -35,7 +35,14 @@ 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; + /** + * From Vulkan spec: + * "The OriginLowerLeft execution mode must not be used; fragment entry + * points must declare OriginUpperLeft." + * + * So at this point origin_upper_left should be true + */ + assert(b->shader->info.fs.origin_upper_left == true); return nir_load_var(b, pos); } diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h index ca00de7dc63..9bca5c153ad 100644 --- a/src/mesa/main/mtypes.h +++ b/src/mesa/main/mtypes.h @@ -2102,10 +2102,6 @@ struct gl_program /** Texture units used for samplerExternalOES */ GLbitfield ExternalSamplersUsed; - /* Fragement shader only fields */ - GLboolean OriginUpperLeft; - GLboolean PixelCenterInteger; - /** Named parameters, constants, etc. from program text */ struct gl_program_parameter_list *Parameters; diff --git a/src/mesa/program/arbprogparse.c b/src/mesa/program/arbprogparse.c index 7cb1beb5bbb..4038e475c92 100644 --- a/src/mesa/program/arbprogparse.c +++ b/src/mesa/program/arbprogparse.c @@ -117,8 +117,8 @@ _mesa_parse_arb_fragment_program(struct gl_context* ctx, GLenum target, program->SamplersUsed |= (1 << i); } program->ShadowSamplers = prog.ShadowSamplers; - program->OriginUpperLeft = state.option.OriginUpperLeft; - program->PixelCenterInteger = state.option.PixelCenterInteger; + program->info.fs.origin_upper_left = state.option.OriginUpperLeft; + program->info.fs.pixel_center_integer = state.option.PixelCenterInteger; program->info.fs.uses_discard = state.fragment.UsesKill; diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp index 2908819d28e..e65a6743353 100644 --- a/src/mesa/program/ir_to_mesa.cpp +++ b/src/mesa/program/ir_to_mesa.cpp @@ -618,11 +618,6 @@ ir_to_mesa_visitor::find_variable_storage(const ir_variable *var) void ir_to_mesa_visitor::visit(ir_variable *ir) { - if (strcmp(ir->name, "gl_FragCoord") == 0) { - this->prog->OriginUpperLeft = ir->data.origin_upper_left; - this->prog->PixelCenterInteger = ir->data.pixel_center_integer; - } - if (ir->data.mode == ir_var_uniform && strncmp(ir->name, "gl_", 3) == 0) { unsigned int i; const ir_state_slot *const slots = ir->get_state_slots(); diff --git a/src/mesa/program/prog_to_nir.c b/src/mesa/program/prog_to_nir.c index 312b299361e..1c9d0018d55 100644 --- a/src/mesa/program/prog_to_nir.c +++ b/src/mesa/program/prog_to_nir.c @@ -890,10 +890,7 @@ setup_registers_and_variables(struct ptn_compile *c) var->data.index = 0; 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; - } else if (i == VARYING_SLOT_FOGC) { + if (i == VARYING_SLOT_FOGC) { /* fogcoord is defined as . Make the actual * input variable a float, and create a local containing the * full vec4 value. @@ -934,12 +931,6 @@ setup_registers_and_variables(struct ptn_compile *c) var->data.location = i; var->data.index = 0; - 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; - } - c->sysval_vars[i] = var; } diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp index 2102b7a57d5..484a5329455 100644 --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp @@ -1136,11 +1136,6 @@ glsl_to_tgsi_visitor::find_variable_storage(ir_variable *var) void glsl_to_tgsi_visitor::visit(ir_variable *ir) { - if (strcmp(ir->name, "gl_FragCoord") == 0) { - this->prog->OriginUpperLeft = ir->data.origin_upper_left; - this->prog->PixelCenterInteger = ir->data.pixel_center_integer; - } - if (ir->data.mode == ir_var_uniform && strncmp(ir->name, "gl_", 3) == 0) { unsigned int i; const ir_state_slot *const slots = ir->get_state_slots(); @@ -6422,7 +6417,7 @@ emit_wpos(struct st_context *st, * u,i -> l,h: (99.0 + 0.5) * -1 + 100 = 0.5 * u,h -> l,i: (99.5 + 0.5) * -1 + 100 = 0 */ - if (program->OriginUpperLeft) { + if (program->info.fs.origin_upper_left) { /* Fragment shader wants origin in upper-left */ if (pscreen->get_param(pscreen, PIPE_CAP_TGSI_FS_COORD_ORIGIN_UPPER_LEFT)) { /* the driver supports upper-left origin */ @@ -6449,7 +6444,7 @@ emit_wpos(struct st_context *st, assert(0); } - if (program->PixelCenterInteger) { + if (program->info.fs.pixel_center_integer) { /* Fragment shader wants pixel center integer */ if (pscreen->get_param(pscreen, PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_INTEGER)) { /* the driver supports pixel center integer */ diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.c b/src/mesa/state_tracker/st_mesa_to_tgsi.c index 0ea201fdd6a..03a2dee6778 100644 --- a/src/mesa/state_tracker/st_mesa_to_tgsi.c +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.c @@ -734,7 +734,7 @@ emit_wpos(struct st_context *st, * u,i -> l,h: (99.0 + 0.5) * -1 + 100 = 0.5 * u,h -> l,i: (99.5 + 0.5) * -1 + 100 = 0 */ - if (program->OriginUpperLeft) { + if (program->info.fs.origin_upper_left) { /* Fragment shader wants origin in upper-left */ if (pscreen->get_param(pscreen, PIPE_CAP_TGSI_FS_COORD_ORIGIN_UPPER_LEFT)) { @@ -764,7 +764,7 @@ emit_wpos(struct st_context *st, assert(0); } - if (program->PixelCenterInteger) { + if (program->info.fs.pixel_center_integer) { /* Fragment shader wants pixel center integer */ if (pscreen->get_param(pscreen, PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_INTEGER)) { diff --git a/src/mesa/swrast/s_fragprog.c b/src/mesa/swrast/s_fragprog.c index 8b47e48355b..6fb793fb621 100644 --- a/src/mesa/swrast/s_fragprog.c +++ b/src/mesa/swrast/s_fragprog.c @@ -164,9 +164,9 @@ init_machine(struct gl_context *ctx, struct gl_program_machine *machine, GLfloat *wpos = span->array->attribs[VARYING_SLOT_POS][col]; /* ARB_fragment_coord_conventions */ - if (program->OriginUpperLeft) + if (program->info.fs.origin_upper_left) wpos[1] = ctx->DrawBuffer->Height - 1 - wpos[1]; - if (!program->PixelCenterInteger) { + if (!program->info.fs.pixel_center_integer) { wpos[0] += 0.5F; wpos[1] += 0.5F; }