Re: Using __builtin_expect (likely/unlikely macros)

[Alex Gramiak <agrambot@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Alex Gramiak wrote:
>> Though I don't think that these macros, used sparingly around code such
>> as emacs_abort, would make it harder to read/write/maintain.
>
> These macro calls would not help near calls to emacs_abort, as it should already
> be obvious to a careful human reader that the jump to emacs_abort is the road
> less traveled. (That's also obvious to GCC, since emacs_abort is _Noreturn.)

To human readers, yes, but from what I can tell, GCC is mixed on this.
When applying the following diff that surrounds emacs_abort in
bytecode.c and xdisp.c, the assembly in both applied/unapplied is the
same for bytecode.c, but not for xdisp.c (even without the LIKELY
cases). I tested using gcc -O2 -S.

Also, I tried putting a LIKELY in lisp_h_CHECK_TYPE (seems like a nice
place for one) and that yielded different assembly for fns.c, even
though wrong_type_argument is _Noreturn.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: likely --]
[-- Type: text/x-patch, Size: 6351 bytes --]

diff --git a/src/bytecode.c b/src/bytecode.c
index 40977799bf..ce1a7bd254 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -39,6 +39,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #ifndef BYTE_CODE_SAFE
 # define BYTE_CODE_SAFE false
 #endif
+#define unlikely(expr) (__builtin_expect (expr, 0))
 
 /* Define BYTE_CODE_METER to generate a byte-op usage histogram.  */
 /* #define BYTE_CODE_METER */
@@ -404,7 +405,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
       int op;
       enum handlertype type;
 
-      if (BYTE_CODE_SAFE && ! (stack_base <= top && top < stack_lim))
+      if (unlikely (BYTE_CODE_SAFE && ! (stack_base <= top && top < stack_lim)))
 	emacs_abort ();
 
 #ifdef BYTE_CODE_METER
@@ -664,9 +665,9 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	op_branch:
 	  op -= pc - bytestr_data;
 	op_relative_branch:
-	  if (BYTE_CODE_SAFE
+	  if (unlikely (BYTE_CODE_SAFE
 	      && ! (bytestr_data - pc <= op
-		    && op < bytestr_data + bytestr_length - pc))
+		    && op < bytestr_data + bytestr_length - pc)))
 	    emacs_abort ();
 	  quitcounter += op < 0;
 	  if (!quitcounter)
@@ -1397,7 +1398,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	       number of cases is less, which uses a simple vector for linear
 	       search as the jump table.  */
             Lisp_Object jmp_table = POP;
-	    if (BYTE_CODE_SAFE && !HASH_TABLE_P (jmp_table))
+	    if (unlikely (BYTE_CODE_SAFE && !HASH_TABLE_P (jmp_table)))
               emacs_abort ();
             Lisp_Object v1 = POP;
             ptrdiff_t i;
@@ -1426,7 +1427,7 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	    if (i >= 0)
 	      {
 		Lisp_Object val = HASH_VALUE (h, i);
-		if (BYTE_CODE_SAFE && !FIXNUMP (val))
+		if (unlikely (BYTE_CODE_SAFE && !FIXNUMP (val)))
 		  emacs_abort ();
 		op = XFIXNUM (val);
 		goto op_branch;
@@ -1436,8 +1437,8 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 
 	CASE_DEFAULT
 	CASE (Bconstant):
-	  if (BYTE_CODE_SAFE
-	      && ! (Bconstant <= op && op < Bconstant + const_length))
+	  if (unlikely (BYTE_CODE_SAFE
+                        && ! (Bconstant <= op && op < Bconstant + const_length)))
 	    emacs_abort ();
 	  PUSH (vectorp[op - Bconstant]);
 	  NEXT;
diff --git a/src/xdisp.c b/src/xdisp.c
index a88fc698b8..9872f69cb0 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -344,7 +344,8 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Holds the list (error).  */
 static Lisp_Object list_of_error;
-
+#define UNLIKELY(expr) (__builtin_expect (expr, 0))
+#define LIKELY(expr) (__builtin_expect (expr, 1))
 #ifdef HAVE_WINDOW_SYSTEM
 
 /* Test if overflow newline into fringe.  Called with iterator IT
@@ -8322,7 +8323,7 @@ compute_stop_pos_backwards (struct it *it)
       reseat_1 (it, pos, false);
       compute_stop_pos (it);
       /* We must advance forward, right?  */
-      if (it->stop_charpos <= charpos)
+      if (UNLIKELY (it->stop_charpos <= charpos))
 	emacs_abort ();
     }
   while (charpos > BEGV && it->stop_charpos >= it->end_charpos);
@@ -8371,7 +8372,7 @@ handle_stop_backwards (struct it *it, ptrdiff_t charpos)
 	it->current.string_pos = string_pos (charpos, it->string);
       compute_stop_pos (it);
       /* We must advance forward, right?  */
-      if (it->stop_charpos <= it->prev_stop)
+      if (UNLIKELY (it->stop_charpos <= it->prev_stop))
 	emacs_abort ();
       charpos = it->stop_charpos;
     }
@@ -11443,7 +11444,7 @@ pop_message_unwind (void)
 void
 check_message_stack (void)
 {
-  if (!NILP (Vmessage_stack))
+  if (UNLIKELY (!NILP (Vmessage_stack)))
     emacs_abort ();
 }
 
@@ -15495,7 +15496,7 @@ set_cursor_from_row (struct window *w, struct glyph_row *row,
       /* Need to compute x that corresponds to GLYPH.  */
       for (g = row->glyphs[TEXT_AREA], x = row->x; g < glyph; g++)
 	{
-	  if (g >= row->glyphs[TEXT_AREA] + row->used[TEXT_AREA])
+	  if (UNLIKELY (g >= row->glyphs[TEXT_AREA] + row->used[TEXT_AREA]))
 	    emacs_abort ();
 	  x += g->pixel_width;
 	}
@@ -16827,9 +16828,9 @@ redisplay_window (Lisp_Object window, bool just_this_one_p)
 
   /* Some sanity checks.  */
   CHECK_WINDOW_END (w);
-  if (Z == Z_BYTE && CHARPOS (opoint) != BYTEPOS (opoint))
+  if (UNLIKELY (Z == Z_BYTE && CHARPOS (opoint) != BYTEPOS (opoint)))
     emacs_abort ();
-  if (BYTEPOS (opoint) < CHARPOS (opoint))
+  if (UNLIKELY (BYTEPOS (opoint) < CHARPOS (opoint)))
     emacs_abort ();
 
   if (mode_line_update_needed (w))
@@ -19318,9 +19319,9 @@ try_window_id (struct window *w)
       adjust_window_ends (w, last_text_row, false);
       eassert (w->window_end_bytepos >= 0);
     }
-  else if (first_unchanged_at_end_row == NULL
-	   && last_text_row == NULL
-	   && last_text_row_at_end == NULL)
+  else if (LIKELY (first_unchanged_at_end_row == NULL
+                   && last_text_row == NULL
+                   && last_text_row_at_end == NULL))
     {
       /* Displayed to end of window, but no line containing text was
 	 displayed.  Lines were deleted at the end of the window.  */
@@ -21015,7 +21016,7 @@ find_row_edges (struct it *it, struct glyph_row *row,
 	   which puts the iterator at the beginning of the next line, in
 	   the logical order. */
 	row->maxpos = it->current.pos;
-      else if (max_pos == min_pos && it->method != GET_FROM_BUFFER)
+      else if (LIKELY (max_pos == min_pos && it->method != GET_FROM_BUFFER))
 	/* A line that is entirely from a string/image/stretch...  */
 	row->maxpos = row->minpos;
       else
@@ -25210,7 +25211,7 @@ display_string (const char *string, Lisp_Object lisp_string, Lisp_Object face_st
 		}
 	      break;
 	    }
-	  else if (x + glyph->pixel_width >= it->first_visible_x)
+	  else if (LIKELY (x + glyph->pixel_width >= it->first_visible_x))
 	    {
 	      /* Glyph is at least partially visible.  */
 	      ++it->hpos;
@@ -27847,7 +27848,7 @@ produce_special_glyphs (struct it *it, enum display_element_type what)
 	  spec_glyph_lookup_face (XWINDOW (it->window), &glyph);
 	}
     }
-  else if (what == IT_TRUNCATION)
+  else if (LIKELY (what == IT_TRUNCATION))
     {
       /* Truncation glyph.  */
       SET_GLYPH_FROM_CHAR (glyph, '$');