[Mesa-dev] [PATCH] i965: Fix bitcast operations with negate (ceil)

[Iago Toral]

On Wed, 2014-12-17 at 04:19 -0800, Jason Ekstrand wrote:
> On Wed, Dec 17, 2014 at 1:24 AM, Iago Toral Quiroga
> <itoral at igalia.com> wrote:
>         Commit 0ae9ca12a8 put source modifiers out of the bitcast
>         operations
>         by adding a MOV operation that would handle them separately.
>         It missed
>         the case of ceil though, which negates both its source and
>         destination
>         operands.
>         
>         Fixes the following 42 dEQP tests:
>         dEQP-GLES3.functional.shaders.builtin_functions.common.ceil.*_vertex
>         dEQP-GLES3.functional.shaders.builtin_functions.common.ceil.*_fragment
>         dEQP-GLES3.functional.shaders.builtin_functions.precision.ceil._*vertex.*
>         dEQP-GLES3.functional.shaders.builtin_functions.precision.ceil._*fragment.*
>         ---
>          src/mesa/drivers/dri/i965/brw_fs_visitor.cpp   | 12
>         ++++++++----
>          src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 12
>         ++++++++----
>          2 files changed, 16 insertions(+), 8 deletions(-)
>         
>         diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>         b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>         index b4f8f37..4526b75 100644
>         --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>         +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>         @@ -889,10 +889,14 @@ fs_visitor::visit(ir_expression *ir)
>             case ir_unop_trunc:
>                emit(RNDZ(this->result, op[0]));
>                break;
>         -   case ir_unop_ceil:
>         -      op[0].negate = !op[0].negate;
>         -      emit(RNDD(this->result, op[0]));
>         -      this->result.negate = true;
>         +   case ir_unop_ceil: {
>         +         fs_reg tmp = fs_reg(this, ir->type);
>         +         op[0].negate = !op[0].negate;
>         +         emit(MOV(tmp, op[0]));
> 
> 
> Do we really need this first mov?  I don't think we do as we can just
> use a source modifier on RNDD.  It won't affect the quality of the
> final code because copy propagation should clean it up, but more
> direct codegen is nicer.

Yes, you are right. I'll remove it and send a new patch.
Thanks!

>  
>         +         emit(RNDD(tmp, tmp));
>         +         tmp.negate = true;
>         +         emit(MOV(this->result, tmp));
>         +      }
>                break;
>             case ir_unop_floor:
>                emit(RNDD(this->result, op[0]));
>         diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>         b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>         index 73fff75..7c4a3e1 100644
>         --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>         +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>         @@ -1649,10 +1649,14 @@ vec4_visitor::visit(ir_expression *ir)
>             case ir_unop_trunc:
>                emit(RNDZ(result_dst, op[0]));
>                break;
>         -   case ir_unop_ceil:
>         -      op[0].negate = !op[0].negate;
>         -      inst = emit(RNDD(result_dst, op[0]));
>         -      this->result.negate = true;
>         +   case ir_unop_ceil: {
>         +         src_reg tmp = src_reg(this, ir->type);
>         +         op[0].negate = !op[0].negate;
>         +         emit(MOV(dst_reg(tmp), op[0]));
>         +         emit(RNDD(dst_reg(tmp), tmp));
>         +         tmp.negate = true;
>         +         emit(MOV(result_dst, tmp));
>         +      }
>                break;
>             case ir_unop_floor:
>                inst = emit(RNDD(result_dst, op[0]));
>         --
>         1.9.1
>         
>         _______________________________________________
>         mesa-dev mailing list
>         mesa-dev at lists.freedesktop.org
>         http://lists.freedesktop.org/mailman/listinfo/mesa-dev