[Mesa-dev] [PATCH] ir_to_mesa: Don't optimize clamp into saturate when target is GL_VERTEX_PROGRAM_ARB

[Abdiel Janulgue]

On 11/29/2014 12:10 PM, Kenneth Graunke wrote:
> On Thursday, November 27, 2014 10:45:00 AM Abdiel Janulgue wrote:
>> Add the the same restriction as in the previous try_emit_sat when trying
>> to optimize clamp. Fixes an infinite loop in swrast where the lowering
>> pass unpacks saturate into clamp but the opt_algebraic pass tries to do
>> the opposite.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83463
>>
>> Tested-by: Vinson Lee <vlee at freedesktop.org>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
>> ---
>>  src/glsl/opt_algebraic.cpp      | 3 ++-
>>  src/mesa/main/mtypes.h          | 1 +
>>  src/mesa/program/ir_to_mesa.cpp | 3 ++-
>>  3 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
>> index 430f5cb..62b14dd 100644
>> --- a/src/glsl/opt_algebraic.cpp
>> +++ b/src/glsl/opt_algebraic.cpp
>> @@ -679,7 +679,8 @@ ir_algebraic_visitor::handle_expression(ir_expression *ir)
>>  
>>     case ir_binop_min:
>>     case ir_binop_max:
>> -      if (ir->type->base_type != GLSL_TYPE_FLOAT)
>> +      if (ir->type->base_type != GLSL_TYPE_FLOAT ||
>> +         options->EmitNoSat)
>>           break;
>>  
>>        /* Replace min(max) operations and its commutative combinations with
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 7389baa..199443a 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2990,6 +2990,7 @@ struct gl_shader_compiler_options
>>     GLboolean EmitNoMainReturn;            /**< Emit CONT/RET opcodes? */
>>     GLboolean EmitNoNoise;                 /**< Emit NOISE opcodes? */
>>     GLboolean EmitNoPow;                   /**< Emit POW opcodes? */
>> +   GLboolean EmitNoSat;
>>     GLboolean LowerClipDistance; /**< Lower gl_ClipDistance from float[8] to vec4[2]? */
>>  
>>     /**
>> diff --git a/src/mesa/program/ir_to_mesa.cpp b/src/mesa/program/ir_to_mesa.cpp
>> index 5cd9058..44fd315 100644
>> --- a/src/mesa/program/ir_to_mesa.cpp
>> +++ b/src/mesa/program/ir_to_mesa.cpp
>> @@ -2935,7 +2935,7 @@ _mesa_ir_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
>>  
>>        bool progress;
>>        exec_list *ir = prog->_LinkedShaders[i]->ir;
>> -      const struct gl_shader_compiler_options *options =
>> +      struct gl_shader_compiler_options *options =
>>              &ctx->Const.ShaderCompilerOptions[prog->_LinkedShaders[i]->Stage];
>>  
>>        do {
>> @@ -2949,6 +2949,7 @@ _mesa_ir_link_shader(struct gl_context *ctx, struct gl_shader_program *prog)
>>  				 | ((options->EmitNoPow) ? POW_TO_EXP2 : 0)
>>  				 | ((target == GL_VERTEX_PROGRAM_ARB) ? SAT_TO_CLAMP
>>                                      : 0)));
>> +         options->EmitNoSat = (target == GL_VERTEX_PROGRAM_ARB);
>>  
>>  	 progress = do_lower_jumps(ir, true, true, options->EmitNoMainReturn, options->EmitNoCont, options->EmitNoLoops) || progress;
> 
> NAK.  You don't get to edit ctx->Const.  It should be set up at context
> initialization time (either in core mesa or in the driver), and never
> touched after that.
> 
> You should:
> 1. Create an gl_shader_compiler_options::EmitNoSat field (hunk 2 of this patch).
> 2. Set it appropriately in all relevant drivers:
>    - swrast
>    - r200
>    - i915
>    - i965
>    - Gallium (st_extensions.c)
>    You can skip radeon and nouveau_vieux as they don't support shaders.
> 3. Make opt_algebraic pay attention to it (hunk 1 of this patch).
> 4. Change all callers of lower_instructions with SAT_TO_CLAMP to
>    use options->EmitNoSat, not target == GL_VERTEX_PROGRAM stuff.
> 
> It's unclear to me that we're making the correct decisions today - in other
> words, setting EmitNoSat /properly/ for drivers may be a change in behavior.
> 
> In particular, in commit cfa8c1cb, your st_glsl_to_tgsi.cpp hunk appears to be
> doing SAT_TO_CLAMP lowering only for vertex shaders on pipe drivers that
> *support SM3*.  It seems like you meant to do lowering for ones that
> *don't* support SM3.
> 
> Please try to get this fixed ASAP - we have a release coming up in under a
> week; we can't afford to wait two months to attend to simple regressions.
> 

Will do. Sending v2 now...

> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>