On Mon, May 23, 2016 at 5:12 PM, Dave Airlie <airl...@gmail.com> wrote: > On 24 May 2016 at 06:56, Ilia Mirkin <imir...@alum.mit.edu> wrote: >> On Thu, May 19, 2016 at 11:47 PM, Dave Airlie <airl...@gmail.com> wrote: >>> From: Dave Airlie <airl...@redhat.com> >>> >>> The last version of this broke clipping, and I had to spend >>> sometime getting this working properly. >>> >>> I had to introduce a third pass to count the clip/cull totals, >>> all due to one messy corner case. We have a piglit test >>> tes-input-gl_ClipDistance.shader_test >>> that doesn't actually output the clip distances, it just passes >>> them like a varying from TCS->TES, the older lowering pass worked >>> but to lower clip/cull we need to know the total number of clip+culls >>> used to defined the new variable correctly, and to offset culls >>> properly. >>> >>> This adds an extra pass that works out the sizes for clip/cull, >>> then lowers gl_ClipDistance then gl_CullDistance into the new >>> gl_ClipDistanceMESA. >>> >>> The pass checks using the fixed array sizes code if they array >>> has been referenced, or is actually never used, and ignores >>> it in the latter case. >>> >>> Signed-off-by: Dave Airlie <airl...@redhat.com> >>> --- >>> src/compiler/glsl/ir_optimization.h | 2 +- >>> src/compiler/glsl/linker.cpp | 2 +- >>> src/compiler/glsl/lower_distance.cpp | 220 >>> ++++++++++++++++++++++++++--------- >>> 3 files changed, 164 insertions(+), 60 deletions(-) >>> >>> diff --git a/src/compiler/glsl/ir_optimization.h >>> b/src/compiler/glsl/ir_optimization.h >>> index 5fc2740..71b10e4 100644 >>> --- a/src/compiler/glsl/ir_optimization.h >>> +++ b/src/compiler/glsl/ir_optimization.h >>> @@ -119,7 +119,7 @@ bool >>> lower_variable_index_to_cond_assign(gl_shader_stage stage, >>> bool lower_temp, bool lower_uniform); >>> bool lower_quadop_vector(exec_list *instructions, bool dont_lower_swz); >>> bool lower_const_arrays_to_uniforms(exec_list *instructions); >>> -bool lower_clip_distance(gl_shader *shader); >>> +bool lower_clip_cull_distance(struct gl_shader_program *prog, gl_shader >>> *shader); >>> void lower_output_reads(unsigned stage, exec_list *instructions); >>> bool lower_packing_builtins(exec_list *instructions, int op_mask); >>> void lower_shared_reference(struct gl_shader *shader, unsigned >>> *shared_size); >>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp >>> index b856631..4b5b32c 100644 >>> --- a/src/compiler/glsl/linker.cpp >>> +++ b/src/compiler/glsl/linker.cpp >>> @@ -4639,7 +4639,7 @@ link_shaders(struct gl_context *ctx, struct >>> gl_shader_program *prog) >>> goto done; >>> >>> if >>> (ctx->Const.ShaderCompilerOptions[i].LowerCombinedClipCullDistance) { >>> - lower_clip_distance(prog->_LinkedShaders[i]); >>> + lower_clip_cull_distance(prog, prog->_LinkedShaders[i]); >>> } >>> >>> if (ctx->Const.LowerTessLevel) { >>> diff --git a/src/compiler/glsl/lower_distance.cpp >>> b/src/compiler/glsl/lower_distance.cpp >>> index 301afe4..f21a1be 100644 >>> --- a/src/compiler/glsl/lower_distance.cpp >>> +++ b/src/compiler/glsl/lower_distance.cpp >>> @@ -45,19 +45,41 @@ >>> * LowerCombinedClipCullDistance flag in gl_shader_compiler_options to >>> true. >>> */ >>> >>> +#include "main/macros.h" >>> #include "glsl_symbol_table.h" >>> #include "ir_rvalue_visitor.h" >>> #include "ir.h" >>> #include "program/prog_instruction.h" /* For WRITEMASK_* */ >>> >>> +#define GLSL_CLIP_VAR_NAME "gl_ClipDistanceMESA" >>> + >>> namespace { >>> >>> class lower_distance_visitor : public ir_rvalue_visitor { >>> public: >>> - explicit lower_distance_visitor(gl_shader_stage shader_stage) >>> + explicit lower_distance_visitor(gl_shader_stage shader_stage, >>> + const char *in_name, int total_size, >>> + int offset) >>> : progress(false), old_distance_out_var(NULL), >>> old_distance_in_var(NULL), new_distance_out_var(NULL), >>> - new_distance_in_var(NULL), shader_stage(shader_stage) >>> + new_distance_in_var(NULL), shader_stage(shader_stage), >>> + in_name(in_name), total_size(total_size), offset(offset) >>> + { >>> + } >>> + >>> + explicit lower_distance_visitor(gl_shader_stage shader_stage, >>> + const char *in_name, >>> + const lower_distance_visitor *orig, >>> + int offset) >>> + : progress(false), >>> + old_distance_out_var(NULL), >>> + old_distance_in_var(NULL), >>> + new_distance_out_var(orig->new_distance_out_var), >>> + new_distance_in_var(orig->new_distance_in_var), >>> + shader_stage(shader_stage), >>> + in_name(in_name), >>> + total_size(orig->total_size), >>> + offset(offset) >> >> Please don't mix tabs and spaces. Seems like this has a healthy mix >> with every other line alternating style... > > remote emacs editing, doesn't always pick up the style files, will fix that. >>> >>> - if (!ir->type->fields.array->is_array()) { >>> - /* gl_ClipDistance (used for vertex, tessellation evaluation and >>> - * geometry output, and fragment input). >>> - */ >>> - assert((ir->data.mode == ir_var_shader_in && >>> - this->shader_stage == MESA_SHADER_FRAGMENT) || >>> - (ir->data.mode == ir_var_shader_out && >>> - (this->shader_stage == MESA_SHADER_VERTEX || >>> - this->shader_stage == MESA_SHADER_TESS_EVAL || >>> - this->shader_stage == MESA_SHADER_GEOMETRY))); >>> + *old_var = ir; >>> >>> - *old_var = ir; >>> - assert (ir->type->fields.array == glsl_type::float_type); >>> - unsigned new_size = (ir->type->array_size() + 3) / 4; >>> + if (!(*new_var)) { >> >> This is a new check, the old code didn't have. When is *new_var going >> to be null? >> > > Always at the start, the old code always assigned it, since it was all done > in a single pass, since we do two passes once for clip, once for cull, > but we only ever want to define one new_var (gl_ClipDistanceMESA).
Oh right, that makes sense. So if you've already defined the new var, just remove the new thing. + (*new_var)->data.max_array_access = ir->data.max_array_access / 4; That's wrong in both sides of the if though, no? Shouldn't it just be new_size - 1 for the non-primitive-input case? And I don't even know what it should be in the primitive input case since the outer array should stay the same while the inner array should have new_size - 1. > >>> + cull_size = &out_cull_size; >>> + } else if (ir->data.mode == ir_var_shader_in) { >>> + clip_size = &in_clip_size; >>> + cull_size = &in_cull_size; >>> + } else >>> + return visit_continue; >>> + >>> + if (ir->type->is_unsized_array()) >> >> || !ir->type->is_array() presumably? If some jerk did "float >> gl_CullDistance", the below would go rather poorly... > > I don't think the compiler lets you do float gl_ClipDistance > or gl_CullDistance. It's definitely not something I need to care > about at this depth. How about asserting then? > >> Would it be cleaner to identify variables by location rather than >> name? e.g. what happens if I don't have cull enabled, and use that as >> a regular ol' varying name. Whereas the only way to have a >> VARYING_SLOT_CULL position is to go via the proper built-in path. > > gl_ names aren't user useable, the compiler would block it. Hm ok. I'd still prefer avoiding strcmp, but I'll leave it to you. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev