[Mesa-dev] [PATCH] mesa, glsl_to_tgsi: fixes for native integers

[Christoph Bumiller]

On 29.08.2011 19:22, Ian Romanick wrote:
> On 08/27/2011 08:44 AM, Bryan Cain wrote:
> > On 08/27/2011 05:39 AM, Christoph Bumiller wrote:
> >> On 27.08.2011 04:58, Bryan Cain wrote:
> >>> This fixes all of the piglit regressions in softpipe when native
> integers are
> >>> enabled.
> >>> ---
> >>>  src/mesa/main/uniforms.c                   |    8 +----
> >>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp |   45
> ++++++++++++++++++++++++++--
> >>>  2 files changed, 43 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/src/mesa/main/uniforms.c b/src/mesa/main/uniforms.c
> >>> index cda840f..0801476 100644
> >>> --- a/src/mesa/main/uniforms.c
> >>> +++ b/src/mesa/main/uniforms.c
> >>> @@ -776,13 +776,7 @@ set_program_uniform(struct gl_context *ctx,
> struct gl_program *program,
> >>>           /* if the uniform is bool-valued, convert to 1 or 0 */
> >>>           if (isUniformBool) {
> >>>              for (i = 0; i < elems; i++) {
> >>> -               if (basicType == GL_FLOAT)
> >>> -                  uniformVal[i].b = uniformVal[i].f != 0.0f ? 1 : 0;
> >>> -               else
> >>> -                  uniformVal[i].b = uniformVal[i].u ? 1 : 0;
> >>> -               
> >>> -               if (!ctx->Const.NativeIntegers)
> >>> -                  uniformVal[i].f = uniformVal[i].b ? 1.0f : 0.0f;
> >>> +               uniformVal[i].f = uniformVal[i].f != 0.0f ? 1.0f :
> 0.0f;
> >>>              }
> >>>           }
> >>>        }
> >>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> index 9cac309..d1674eb 100644
> >>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> >>> @@ -385,6 +385,8 @@ public:
> >>>     void emit_scalar(ir_instruction *ir, unsigned op,
> >>>                  st_dst_reg dst, st_src_reg src0, st_src_reg src1);
> >>>  
> >>> +   void try_emit_integer_set(ir_instruction *ir, unsigned op,
> st_dst_reg dst);
> >>> +
> >>>     void emit_arl(ir_instruction *ir, st_dst_reg dst, st_src_reg
> src0);
> >>>  
> >>>     void emit_scs(ir_instruction *ir, unsigned op,
> >>> @@ -563,6 +565,8 @@ glsl_to_tgsi_visitor::emit(ir_instruction *ir,
> unsigned op,
> >>>  
> >>>     this->instructions.push_tail(inst);
> >>>     
> >>> +   try_emit_integer_set(ir, op, dst);
> >>> +   
> >>>     return inst;
> >>>  }
> >>>  
> >>> @@ -589,6 +593,32 @@ glsl_to_tgsi_visitor::emit(ir_instruction
> *ir, unsigned op)
> >>>  }
> >>>  
> >>>  /**
> >>> + * Emits the code to convert the result of integer SET
> instructions to floats.
> >>> + */
> >>> +void
> >>> +glsl_to_tgsi_visitor::try_emit_integer_set(ir_instruction *ir,
> unsigned op,
> >>> +                 st_dst_reg dst)
> >>> +{
> >>> +   st_src_reg src;
> >>> +
> >>> +   if (!(op == TGSI_OPCODE_USEQ ||
> >>> +         op == TGSI_OPCODE_USNE ||
> >>> +         op == TGSI_OPCODE_ISGE ||
> >>> +         op == TGSI_OPCODE_USGE ||
> >>> +         op == TGSI_OPCODE_ISLT ||
> >>> +         op == TGSI_OPCODE_USLT))
> >>> +       return;
> >>> +
> >>> +   src = st_src_reg(dst);
> >>> +   src.type = GLSL_TYPE_INT;
> >>> +
> >>> +   dst.type = GLSL_TYPE_FLOAT;
> >>> +   emit(ir, TGSI_OPCODE_I2F, dst, src);
> >>> +   src.type = GLSL_TYPE_FLOAT;
> >>> +   emit(ir, TGSI_OPCODE_SNE, dst, src, st_src_reg_for_float(0.0));
> >>> +}
> >>> +
> >> emit(ir, TGSI_OPCODE_AND, dst, src, st_src_reg_for_float(1.0f));
> >>
> >> I still don't quite like booleans as floats, but I guess I can just
> >> easily track back to the source SET to emit my
> set-predicate-register op
> >> without having all the fuss in between.
>
> > For now, I'm trying to get things working.  Once integers are working on
> > softpipe and r600g with no piglit regressions, I'll look into
> > optimizations like using AND.
>
> I think this is the underlying problem.  Without this patch series, some
> of the code treats booleans as integers, and some of it still tries to
> apply floating-point tricks.  Instead of reverting back to using
> floating-point for booleans, it seems much better (by all metrics) to
> finish the conversion to integer.  Emitting (1-x) for logical-not or
> (a*b) for logical-and when you have an actual NOT and AND instructions
> is just silly.
>
> Fixing that will make it irrelevant whether the integer set-on
> instructions generate 1 or ~0 for true.  Right?
>
The NOT will only work for ~0 but not 1, so it does matter.

So should we force everyone to generate a specific value (~0 or 1, i.e.
some hardware will have to NEG, which is mostly free) for integer SET,
and emit the F2I(NEG(b)) for float SET (except for e.g. GLSL step()) ?

With ~0 you can do NOT, with 1 you don't need negation on cast to int,
or with 1.0f you don't need to do anything on cast to float (seems like
a rare case to me) but use AND after every integer SET.

There's no optimal solution, you can only hope to optimize away any
extra operations e.g. if you see IF(F2I(NEG(SET()))), you know that F2I
and NEG don't make a difference so you let the SET write your
FLAGS/PREDICATE register(s) and kill the other operations.

> >>> +/**
> >>>   * Determines whether to use an integer, unsigned integer, or
> float opcode
> >>>   * based on the operands and input opcode, then emits the result.
> >>>   *
> >>> @@ -1662,7 +1692,6 @@ glsl_to_tgsi_visitor::visit(ir_expression *ir)
> >>>        emit_scalar(ir, TGSI_OPCODE_RSQ, result_dst, op[0]);
> >>>        break;
> >>>     case ir_unop_i2f:
> >>> -   case ir_unop_b2f:
> >>>        if (native_integers) {
> >>>           emit(ir, TGSI_OPCODE_I2F, result_dst, op[0]);
> >>>           break;
> >>> @@ -1670,10 +1699,14 @@ glsl_to_tgsi_visitor::visit(ir_expression *ir)
> >>>     case ir_unop_i2u:
> >>>     case ir_unop_u2i:
> >>>        /* Converting between signed and unsigned integers is a
> no-op. */
> >>> -   case ir_unop_b2i:
> >>> -      /* Booleans are stored as integers (or floats in GLSL 1.20
> and lower). */
> >>> +   case ir_unop_b2f:
> >>> +      /* Booleans are stored as floats. */
> >>>        result_src = op[0];
> >>>        break;
> >>> +   case ir_unop_b2i:
> >>> +      if (native_integers)
> >>> +         emit(ir, TGSI_OPCODE_F2I, result_dst, op[0]);
> >>> +      break;
> >>>     case ir_unop_f2i:
> >>>        if (native_integers)
> >>>           emit(ir, TGSI_OPCODE_F2I, result_dst, op[0]);
> >>> @@ -1681,6 +1714,11 @@ glsl_to_tgsi_visitor::visit(ir_expression *ir)
> >>>           emit(ir, TGSI_OPCODE_TRUNC, result_dst, op[0]);
> >>>        break;
> >>>     case ir_unop_f2b:
> >>> +      if (native_integers) {
> >>> +         emit(ir, TGSI_OPCODE_SNE, result_dst, op[0],
> st_src_reg_for_float(0.0));
> >>> +         emit(ir, TGSI_OPCODE_F2I, result_dst, result_src);
> >> This is inconsistent, why do you convert to integer here if you store
> >> all booleans as floats ?
> >>
> >>> +      }
> >>> +      break;
> >>>     case ir_unop_i2b:
> >>>        emit(ir, TGSI_OPCODE_SNE, result_dst, op[0],
> >>>              st_src_reg_for_type(result_dst.type, 0));
> >> This can go wrong if the integer has the value of NaN and the
> >> hardware/backend does not use an unordered comparison.
> >>
> >> nv50 is using unordered, x86/gcc also uses >u<comiss by default, r600
> >> docs don't say what the result of SETNE with NaN operand is so good
> luck.
> >>
>
> > That line of code emits USNE, not SNE.  See the get_opcode() function.
>
> >>> @@ -2154,6 +2192,7 @@ glsl_to_tgsi_visitor::visit(ir_assignment *ir)
> >>>        inst = (glsl_to_tgsi_instruction
> *)this->instructions.get_tail();
> >>>        new_inst = emit(ir, inst->op, l, inst->src[0],
> inst->src[1], inst->src[2]);
> >>>        new_inst->saturate = inst->saturate;
> >>> +      inst->dead_mask = inst->dst.writemask;
> >>>     } else {
> >>>        for (i = 0; i < type_size(ir->lhs->type); i++) {
> >>>           emit(ir, TGSI_OPCODE_MOV, l, r);