[Mesa-dev] [PATCH] glsl: disable varying packing when its not safe
Matt Turner
mattst88 at gmail.com
Mon Feb 1 18:25:42 UTC 2016
Bunch of spelling nits, etc.
On Thu, Jan 28, 2016 at 9:52 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> In GLES 3.1+ and GL 4.4+ there is no guarantee that interpolation
> qualifiers will match between stages so we cannot safely pack
> varyings using the current packing pass in Mesa.
>
> We also disable packing on outward facing SSO as these too are outside
> the rule that guarantees the interpolation qualifiers match.
>
> We do however enable packing on individual arrays, structs, and
> matrices as these are required by the transform feedback code and it
> is still safe to do so.
>
> Finally we also enable packing when a varying is only used for
> transform feedback and its not a SSO.
>
> This fixes all remaining rendering issues with the dEQP SSO tests,
> the only issues remaining with thoses tests are to do with validation.
>
> Note: There is still one remaining SSO bug that this patch doesn't fix.
> Their is a chance that VS -> TCS will have mismatching interfaces
> because we pack VS output in case its used by transform feedback but
> don't pack TCS input for performance reasons. This patch will make the
> situation better but doesn't fix it.
>
> Cc: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> ---
>
> This patch applies on top of my ARB_enhanced_layouts component packing
> series [1].
>
> The GLES 3.1 and GL 4.4 changes were tested by changing the code to
> always disable packing and running the changes in Intel CI system.
>
> This patch would also make it possible for a backend to do less packing
> in GLSL IR if it wished to do so now that the transform feedback requirements
> have been isolated.
>
> [1] http://patchwork.freedesktop.org/series/2101/
>
> src/compiler/glsl/ir.h | 7 +
> src/compiler/glsl/ir_optimization.h | 2 +-
> src/compiler/glsl/link_varyings.cpp | 199 +++++++++++++++++++++-------
> src/compiler/glsl/lower_packed_varyings.cpp | 21 ++-
> 4 files changed, 174 insertions(+), 55 deletions(-)
>
> diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
> index 287c0cb..8cadf34 100644
> --- a/src/compiler/glsl/ir.h
> +++ b/src/compiler/glsl/ir.h
> @@ -736,6 +736,13 @@ public:
> unsigned is_unmatched_generic_inout:1;
>
> /**
> + * Is this varying used only by transform feedback?
> + *
> + * This is used by the linker to decide if its safe to pack the varying.
> + */
> + unsigned is_xfb_only: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
> * accessing components. For example, an offset of 1 means that the x
> diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h
> index 94658bb..1ca26cd 100644
> --- a/src/compiler/glsl/ir_optimization.h
> +++ b/src/compiler/glsl/ir_optimization.h
> @@ -129,7 +129,7 @@ void lower_packed_varyings(void *mem_ctx,
> unsigned locations_used, ir_variable_mode mode,
> gl_shader *shader, unsigned base_location,
> bool disable_varying_packing,
> - bool has_enhanced_layouts);
> + bool xfb_enabled, bool has_enhanced_layouts);
> bool lower_vector_insert(exec_list *instructions, bool lower_nonconstant_index);
> bool lower_vector_derefs(gl_shader *shader);
> void lower_named_interface_blocks(void *mem_ctx, gl_shader *shader);
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 78eb2b6..2716723 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -41,6 +41,23 @@
>
>
> /**
> + * Packing is always safe on individual arrays, structure and matices. It is
> + * also safe if the varying is only used for transform feedback.
> + */
> +static bool
> +is_varying_packing_safe(const ir_variable *var,
> + const gl_shader_stage consumer_stage,
> + const gl_shader_stage producer_stage) {
> + if ((consumer_stage == MESA_SHADER_TESS_EVAL) ||
> + (consumer_stage == MESA_SHADER_TESS_CTRL) ||
> + (producer_stage == MESA_SHADER_TESS_CTRL)) {
Extra parentheses.
> + return false;
> +}
> + return var->type->is_array() || var->type->is_record() ||
> + var->type->is_matrix() || var->data.is_xfb_only;
> +}
> +
> +/**
> * Get the varying type stripped of the outermost array if we're processing
> * a stage whose varyings are arrays indexed by a vertex number (such as
> * geometry shader inputs).
> @@ -878,7 +895,7 @@ namespace {
> class varying_matches
> {
> public:
> - varying_matches(bool disable_varying_packing,
> + varying_matches(bool disable_varying_packing, bool xfb_enabled,
> gl_shader_stage producer_stage,
> gl_shader_stage consumer_stage);
> ~varying_matches();
> @@ -892,10 +909,22 @@ private:
> * If true, this driver disables varying packing, so all varyings need to
> * be aligned on slot boundaries, and take up a number of slots equal to
> * their number of matrix columns times their array size.
> + *
> + * Packing may also be disabled because our current packing method is not
> + * safe in SSO or versions of OpenGL where interpolation qualifiers are not
> + * guaranteed to match across stages.
> */
> const bool disable_varying_packing;
>
> /**
> + * If true, this driver has transform feedback enabled. The transform
> + * feedback code requires at least some packing be done even when varying
> + * packing is disabled, fortunately the cases its required for safe its
Doesn't parse. Also "it's"
> + * still to do so. See is_varying_packing_safe().
> + */
> + const bool xfb_enabled;
> +
> + /**
> * Enum representing the order in which varyings are packed within a
> * packing class.
> *
> @@ -909,11 +938,13 @@ private:
> PACKING_ORDER_VEC2,
> PACKING_ORDER_SCALAR,
> PACKING_ORDER_VEC3,
> + PACKING_ORDER_LAST,
> };
>
> static unsigned compute_packing_class(const ir_variable *var);
> static packing_order_enum compute_packing_order(const ir_variable *var);
> static int match_comparator(const void *x_generic, const void *y_generic);
> + static int xfb_comparator(const void *x_generic, const void *y_generic);
>
> /**
> * Structure recording the relationship between a single producer output
> @@ -969,9 +1000,11 @@ private:
> } /* anonymous namespace */
>
> varying_matches::varying_matches(bool disable_varying_packing,
> + bool xfb_enabled,
> gl_shader_stage producer_stage,
> gl_shader_stage consumer_stage)
> : disable_varying_packing(disable_varying_packing),
> + xfb_enabled(xfb_enabled),
> producer_stage(producer_stage),
> consumer_stage(consumer_stage)
> {
> @@ -1067,7 +1100,9 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
> = this->compute_packing_class(var);
> this->matches[this->num_matches].packing_order
> = this->compute_packing_order(var);
> - if (this->disable_varying_packing) {
> + if (this->disable_varying_packing &&
> + !(xfb_enabled &&
> + is_varying_packing_safe(var, consumer_stage, producer_stage))) {
> unsigned slots = type->count_attribute_slots(false);
> this->matches[this->num_matches].num_components = slots * 4;
> } else {
> @@ -1093,37 +1128,24 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
> uint64_t reserved_slots,
> bool separate_shader)
> {
> - /* We disable varying sorting for separate shader programs for the
> - * following reasons:
> - *
> - * 1/ All programs must sort the code in the same order to guarantee the
> - * interface matching. However varying_matches::record() will change the
> - * interpolation qualifier of some stages.
> - *
> - * 2/ GLSL version 4.50 removes the matching constrain on the interpolation
> - * qualifier.
> - *
> - * From Section 4.5 (Interpolation Qualifiers) of the GLSL 4.40 spec:
> - *
> - * "The type and presence of interpolation qualifiers of variables with
> - * the same name declared in all linked shaders for the same cross-stage
> - * interface must match, otherwise the link command will fail.
> - *
> - * When comparing an output from one stage to an input of a subsequent
> - * stage, the input and output don't match if their interpolation
> - * qualifiers (or lack thereof) are not the same."
> - *
> - * "It is a link-time error if, within the same stage, the interpolation
> - * qualifiers of variables of the same name do not match."
> + /* If packing has been disbaled then we cannot safely sort the varyings as
disabled
> + * it may mean we are using a version of OpenGL where interpolation
> + * qualifiers are not guaranteed to be matching across shaders, sorting
> + * in this case could result in mismatching shader interfaces.
> */
> - if (!separate_shader) {
> + if (!this->disable_varying_packing) {
> /* Sort varying matches into an order that makes them easy to pack. */
> qsort(this->matches, this->num_matches, sizeof(*this->matches),
> &varying_matches::match_comparator);
> + } else if (xfb_enabled) {
> + /* Only sort varyings that are only used by transform feedback. */
> + qsort(this->matches, this->num_matches, sizeof(*this->matches),
> + &varying_matches::xfb_comparator);
> }
>
> unsigned generic_location = 0;
> unsigned generic_patch_location = MAX_VARYING*4;
> + bool previous_var_xfb_only = false;
>
> for (unsigned i = 0; i < this->num_matches; i++) {
> unsigned *location = &generic_location;
> @@ -1147,16 +1169,30 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
> /* Advance to the next slot if this varying has a different packing
> * class than the previous one, and we're not already on a slot
> * boundary.
> + *
> + * Also advance to the next slot if packing disabled, this makes sure we
packing /is/ disabled <period>
> + * don't assign varyings the same locations, this is possible because
s/, t/. T/
More information about the mesa-dev
mailing list