[PATCH] Fix __builtin_expect (setjmp (buf) == 0, 1)) generates broken code

Jakub Jelinek jakub@redhat.com
Thu Mar 20 13:12:00 GMT 2003


On Thu, Mar 20, 2003 at 12:20:20PM +0100, Jakub Jelinek wrote:
> I agree with Zack here. __builtin_expect is not part of the C99 standard
> and thus it is up to us how we define it. And IMHO it should just set
> branch probablity, nothing else.
> 
> FYI, while:
>   if (__builtin_expect (setjmp (b) == 0, 1))
>     bar ();
> doesn't work,
>   if (__builtin_expect (setjmp (b), 0) == 0)
>     bar ();
> seems to work just fine, which leads to another thing:
> 
> int baz (void);
> void bar (void);
> void foo (void)
> {
>   if (__builtin_expect (baz (), 0) == 0)
>     bar ();
> }
> 
> vs.
> 
> int baz (void);
> void bar (void);
> void foo (void)
> {
>   if (__builtin_expect (baz () == 0, 1))
>     bar ();
> }
> 
> (this time no messing with functions returning twice etc.), generates
> the exact same assembly when optimizing on IA-32, x86-64, IA-64, sparc -m32,
> but generates worse code for the second variant on s390, s390x, sparc -m64.
> 
> When __builtin_expect makes the code worse rather then better is somewhat
> disturbing (but the first form really cannot be used everywhere, it requires
> that first builtin expect argument is integral type and the expected
> return value is constant).

Fixed thusly:
The problem seems to be that unsafe_for_reeval returned 1 for baz () == 0
and thus baz () == 0 was surrounded into UNSAVE_EXPR. But do_jump didn't
handle UNSAVE_EXPR specially, so it did expand_expr on the UNSAVE_EXPR
forcing baz () == 0 into a temporary and then did a conditional jummp based
on that.
Ok to commit if testing succeeds? What about 3.2/3.3 (with the obvious
changes (expr.c instead of dojump.c in both and unsave_expr_now instead
of (*lang_hooks.unsave_expr_now) on 3.2))?

2003-03-20  Jakub Jelinek  <jakub@redhat.com>

	* dojump.c (do_jump): Handle UNSAVE_EXPR specially.

--- gcc/dojump.c.jj	2003-03-13 11:05:23.000000000 -0500
+++ gcc/dojump.c	2003-03-20 07:52:49.000000000 -0500
@@ -160,6 +160,12 @@ do_jump (exp, if_false_label, if_true_la
       break;
 #endif
 
+    case UNSAVE_EXPR:
+      do_jump (TREE_OPERAND (exp, 0), if_false_label, if_true_label);
+      TREE_OPERAND (exp, 0)
+	= (*lang_hooks.unsave_expr_now) (TREE_OPERAND (exp, 0));
+      break;
+
     case NOP_EXPR:
       if (TREE_CODE (TREE_OPERAND (exp, 0)) == COMPONENT_REF
           || TREE_CODE (TREE_OPERAND (exp, 0)) == BIT_FIELD_REF


	Jakub



More information about the Gcc-patches mailing list