[Mesa-dev] [PATCH] i965: Fix if-statements with boolean comparisons on Sandybridge.

[Paul Berry]

On 15 September 2012 21:25, Kenneth Graunke <kenneth at whitecape.org> wrote:

> In the past, we stored booleans as integer 0 or 1.  At some point, we
> changed to storing them as 0 or some non-zero value.


Nit-pick: I'm assuming this means "0 is treated as false, and all nonzero
values are treated as true".  But that can't be right, since
resolve_bool_comparison() computes (x & 1).  Do we only use a subset of the
possible nonzero values to represent true?  (If so it would be nice to
document that in resolve_bool_comparison())  Or is
resolve_bool_comparison() wrong?

In any case this change seems correct, so:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>

Note: I believe the case for ir_binop_logic_xor (in the same function) also
needs fixing, since it treats "if (x ^ y)" as "if (x != y)".  That won't
work if x and y use different representations of true.


>  The Sandybridge
> specific code for emitting IF instructions with embedded conditional
> modifiers was not updated to account for this change.
>
> For code such as
>
>    if (bool_a == bool_b)
>
> we cannot simply emit an equality comparison: even if both operands are
> true, they may be represented by two different non-zero values.  We must
> first normalize them to 0 or 1 so that they can be compared properly.
>
> Fixes piglit test spec/glsl-1.10/execution/fs-bool-less-compare-true.
> Fixes at least 9 subcases of oglconform's shad-compiler test:
> - TestLessThanEquali
> - TestGreaterThan
> - TestGreaterThani
> - TestGreaterThanEqual
> - TestGreaterThanEquali
> - TestEqualb
> - TestNotEqual
> - TestNotEquali
> - TestNotEqualb
>
> NOTE: This is a candidate for the 9.0 branch (but not 8.0).
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54709
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48629
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index d3cbde3..320e2f0 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1650,6 +1650,9 @@ fs_visitor::emit_if_gen6(ir_if *ir)
>        case ir_binop_all_equal:
>        case ir_binop_nequal:
>        case ir_binop_any_nequal:
> +        resolve_bool_comparison(expr->operands[0], &op[0]);
> +        resolve_bool_comparison(expr->operands[1], &op[1]);
> +
>          inst = emit(BRW_OPCODE_IF, reg_null_d, op[0], op[1]);
>          inst->conditional_mod =
>             brw_conditional_for_comparison(expr->operation);
> --
> 1.7.11.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120917/cc124290/attachment.html>