On 15 September 2012 21:25, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.</blockquote><div> 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 <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> 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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=54709" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=54709</a> Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=48629 Signed-off-by" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=48629 Signed-off-by</a>: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> --- 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div>