nir, glsl: move pixel_center_integer/origin_upper_left to shader_info.fs
authorAlejandro Piñeiro <apinheiro@igalia.com>
Thu, 7 Feb 2019 17:43:58 +0000 (18:43 +0100)
committerAlejandro Piñeiro <apinheiro@igalia.com>
Thu, 21 Feb 2019 10:47:59 +0000 (11:47 +0100)
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

24 files changed:
src/compiler/glsl/ast_to_hir.cpp
src/compiler/glsl/glsl_to_nir.cpp
src/compiler/glsl/ir.cpp
src/compiler/glsl/ir.h
src/compiler/glsl/linker.cpp
src/compiler/nir/nir.h
src/compiler/nir/nir_lower_system_values.c
src/compiler/nir/nir_lower_wpos_ytransform.c
src/compiler/shader_info.h
src/compiler/spirv/spirv_to_nir.c
src/compiler/spirv/vtn_private.h
src/compiler/spirv/vtn_variables.c
src/gallium/drivers/radeonsi/si_shader_nir.c
src/intel/blorp/blorp_blit.c
src/intel/blorp/blorp_clear.c
src/intel/blorp/blorp_nir_builder.h
src/intel/vulkan/anv_nir_lower_input_attachments.c
src/mesa/main/mtypes.h
src/mesa/program/arbprogparse.c
src/mesa/program/ir_to_mesa.cpp
src/mesa/program/prog_to_nir.c
src/mesa/state_tracker/st_glsl_to_tgsi.cpp
src/mesa/state_tracker/st_mesa_to_tgsi.c
src/mesa/swrast/s_fragprog.c

index 620153e6a347d8457c338b2475c89c33f799f3ea..f68ed46435bc87f9b30de9b8147dad6f504206f6 100644 (file)
@@ -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:
index d62de862fac62043c6877141413ce607e380baa4..09a4f19f6f2955cae9b1ded71efd228745848497 100644 (file)
@@ -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) {
index 1d1a56ae9a53714132d0cda4224ff86d1f72d9f2..77e37161b74fbe1cd5348b701106d5a0666ccd53 100644 (file)
@@ -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;
index d05d1998a50739f8937c36d0668f873ddd80a246..fa124863d09b39833679d50613a1331987a1371b 100644 (file)
@@ -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?
        *
index 2d76e852f47490b31dd4736213a89b2a03a41db8..5a950950b7ccbd91bd4ac4f722a89f64e104a884 100644 (file)
@@ -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;
 }
 
 /**
index 94ca6c4646842477c054c9232ab28dd0d3e30cba..190b7af7c28f55bc3a4f934a3d49722148fa31bd 100644 (file)
@@ -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
index 7c1aa5fa80104017a9019b5050b06ef6fbb300f8..68b0ea89c8d52b7fa7e212e18baab2df03251b68 100644 (file)
@@ -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;
       }
index 444e211b6801d06074bf313470a073a392cc4d46..34a4801d66b9c172fbc0eced80deba41a7c63edf 100644 (file)
@@ -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 */
index ea6f9a163759dd67369f62b8b9fe4e6d1b2f5726..8fbdfab1a8e19325d1294175284da359d896b9d6 100644 (file)
@@ -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;
index 8c07542f832edbd0c9e343cceb2f19611075fb32..f5511587be8a087be69b12f0a3794ee6181533ef 100644 (file)
@@ -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:
index 63313034ba6d747b2ae2f3239d31e3ff73b2ad3b..f3d54051885bdf3484a302abaf691336cea138f2 100644 (file)
@@ -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;
index f6b458b7e78cc55eb32a8e8173e4b85743572bd5..51152520bb66afda3fb8af98c34c109819a68e19 100644 (file)
@@ -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;
       }
index f401221526da3040588f0bd6470e3b1d65976f12..812c2172366d7902a7bdbb8d41764e9c74c5fcf6 100644 (file)
@@ -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;
 
index 9bea4ffbe2cd43a8c6a9ebd349b54eb5cd8f531e..f879ec4141f31df7c3acec3bffd340ea7f1b1e75 100644 (file)
@@ -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");
index 1ca0c44835e33328cc370f194342844dcd64b9c8..657f5970d489e49d7c271d2ad4f1225233c0d01d 100644 (file)
@@ -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),
index 28c34d19da6bab5e2d9abc104cdab95dfbc804e8..9664bdbcd2765e4c811e75da6b6b5d9b9cb7705b 100644 (file)
@@ -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);
 }
index 655e58449553136d9bd93e9f2b3182059e450b45..9c6f9f434d752728a4043304f00b82f2ab020658 100644 (file)
@@ -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);
 }
index ca00de7dc6329e33bc6cb73bbb688316803e3e90..9bca5c153adcdf800149eb3cf418ecb5e537b227 100644 (file)
@@ -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;
 
index 7cb1beb5bbb5269e5e4e322714151ff21e3f1ab6..4038e475c92e00a6d539b56eedb4676edf252d61 100644 (file)
@@ -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;
 
index 2908819d28e4fb2b1153cb833d0eae6fa874cc2c..e65a6743353f091268009da3313efc7c3bf3a6c6 100644 (file)
@@ -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();
index 312b299361ed0498ce6f2f609497ef7bfa60ddb6..1c9d0018d556e4ed287f1e24a3c0cb318460f310 100644 (file)
@@ -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 <f, 0.0, 0.0, 1.0>.  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;
    }
 
index 2102b7a57d5b071ec283a34cfe590db8e47b07c9..484a5329455c8d5182661105e3486530bd7849de 100644 (file)
@@ -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 */
index 0ea201fdd6a2ddc94095eae7574f1385151a4c96..03a2dee677813569dfc5a9d1ab0f84bb3fec62f7 100644 (file)
@@ -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)) {
index 8b47e48355bd65845dad23c2b5b187de62024676..6fb793fb62107475d6b610af3a6722b4c5666f1f 100644 (file)
@@ -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;
    }