Redundant type checking in window.c and w32menu.c

Redundant type checking in window.c and w32menu.c

[Dmitry Antipov]

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

If we pass CHECK_CONS(), we don't need CONSP()s in Fcar() and Fcdr() and may use XCAR()
and XCDR() instead.

Dmitry

P.S. Is code size the only reason to call Fcar(), Fcdr() and their safe versions directly
from C code ? Replacing these dumb proxies with an appropriate macros eliminates a lot of
function calls at the cost of ~28K increment in code size (for a stripped binary on x86).
Note if someone needs smaller emacs executable (what a strange requirement, but why not ?),
just replacing -O2 with -Os saves ~235K.

P.P.S. Maintainers, please update my e-mail to dmantipov@yandex.ru in src/ChangeLog -
dmitry.antipov@mail.ru is no longer used.

[-- Attachment #2: omit_redundant_type_check.patch --]
[-- Type: text/plain, Size: 1098 bytes --]

Index: window.c
===================================================================
RCS file: /sources/emacs/emacs/src/window.c,v
retrieving revision 1.577
diff -u -r1.577 window.c
--- window.c	1 Jun 2007 10:57:37 -0000	1.577
+++ window.c	19 Jun 2007 14:05:32 -0000
@@ -942,8 +942,8 @@
   w = XWINDOW (window);
   f = XFRAME (w->frame);
   CHECK_CONS (coordinates);
-  lx = Fcar (coordinates);
-  ly = Fcdr (coordinates);
+  lx = XCAR (coordinates);
+  ly = XCDR (coordinates);
   CHECK_NUMBER_OR_FLOAT (lx);
   CHECK_NUMBER_OR_FLOAT (ly);
   x = FRAME_PIXEL_X_FROM_CANON_X (f, lx) + FRAME_INTERNAL_BORDER_WIDTH (f);
Index: w32menu.c
===================================================================
RCS file: /sources/emacs/emacs/src/w32menu.c,v
retrieving revision 1.90
diff -u -r1.90 w32menu.c
--- w32menu.c	17 Jun 2007 22:10:06 -0000	1.90
+++ w32menu.c	19 Jun 2007 14:05:32 -0000
@@ -605,7 +605,7 @@
       else
 	{
 	  CHECK_CONS (item);
-	  item1 = Fcar (item);
+	  item1 = XCAR (item);
 	  CHECK_STRING (item1);
 	  push_menu_item (item1, Qt, Fcdr (item), Qt, Qnil, Qnil, Qnil, Qnil);
 	}

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Redundant type checking in window.c and w32menu.c

[Stefan Monnier]

> P.S. Is code size the only reason to call Fcar(), Fcdr() and their safe
> versions directly from C code ? Replacing these dumb proxies with an
> appropriate macros eliminates a lot of function calls at the cost of ~28K
> increment in code size (for a stripped binary on x86).

No idea.

> Note if someone needs smaller emacs executable (what a strange
> requirement, but why not ?), just replacing -O2 with -Os saves ~235K.

Interestingly enough, at some point (i.e. some combination of x86 processors
and gcc versions) the -Os was the best speedwise as well (at least, it was
used for many SPEC submissions).


        Stefan

Re: Redundant type checking in window.c and w32menu.c

[Ken Raeburn]

On Jun 19, 2007, at 10:44, Dmitry Antipov wrote:
> If we pass CHECK_CONS(), we don't need CONSP()s in Fcar() and Fcdr 
> () and may use XCAR()
> and XCDR() instead.

Makes sense.

> P.S. Is code size the only reason to call Fcar(), Fcdr() and their  
> safe versions directly
> from C code ? Replacing these dumb proxies with an appropriate  
> macros eliminates a lot of
> function calls at the cost of ~28K increment in code size (for a  
> stripped binary on x86).
> Note if someone needs smaller emacs executable (what a strange  
> requirement, but why not ?),
> just replacing -O2 with -Os saves ~235K.

You could make Fcar a static inline function in lisp.h (conditional  
on GCC, or maybe C99).  If the optimizer's good at its job, it should  
eliminate the redundant CONSP checks.  Using an inline function  
avoids having to check all Fcar uses for arguments that have function  
calls or side effects.  (A quick grep shows several of those.  In  
fact, if your test used the simple macro version, inline functions  
may result in less code size expansion because of this.)  And  
personally, I think inline functions are often more readable than  
macros, if they're not very simple macros.

Ken

Re: Redundant type checking in window.c and w32menu.c

[Dmitry Antipov]

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

Ken Raeburn wrote:

> On Jun 19, 2007, at 10:44, Dmitry Antipov wrote:
>> If we pass CHECK_CONS(), we don't need CONSP()s in Fcar() and Fcdr() 
>> and may use XCAR()
>> and XCDR() instead.
> 
> Makes sense.

Whoops, probably not for GNU C :-). At least with -O1 and -O2, GCC (I've
tested with 4.1.2) emits an almost identical and optimal (i.e. with the
only branch ends with a call of wrong_type_argument ()) code for both cases

CHECK_CONS (obj);
x = CAR (obj);
y = CDR (obj);

and

CHECK_CONS (obj);
x = XCAR (obj);
y = XCDR (obj);

Have no ideas about other compilers.

> You could make Fcar a static inline function in lisp.h (conditional on 
> GCC, or maybe C99).  If the optimizer's good at its job, it should 
> eliminate the redundant CONSP checks.  Using an inline function avoids 
> having to check all Fcar uses for arguments that have function calls or 
> side effects.  (A quick grep shows several of those.  In fact, if your 
> test used the simple macro version, inline functions may result in less 
> code size expansion because of this.)  And personally, I think inline 
> functions are often more readable than macros, if they're not very 
> simple macros.

I agree, and here is the stuff I'm trying now. Reordering of data.c
looks ugly, but I have no ideas on how to avoid them :-(.

Dmitry

[-- Attachment #2: inline_cons_fns.patch --]
[-- Type: text/plain, Size: 4317 bytes --]

Index: data.c
===================================================================
RCS file: /sources/emacs/emacs/src/data.c,v
retrieving revision 1.271
diff -u -r1.271 data.c
--- data.c	26 May 2007 17:21:14 -0000	1.271
+++ data.c	20 Jun 2007 14:11:00 -0000
@@ -506,49 +506,6 @@
   return Qnil;
 }
 
-\f
-/* Extract and set components of lists */
-
-DEFUN ("car", Fcar, Scar, 1, 1, 0,
-       doc: /* Return the car of LIST.  If arg is nil, return nil.
-Error if arg is not nil and not a cons cell.  See also `car-safe'.
-
-See Info node `(elisp)Cons Cells' for a discussion of related basic
-Lisp concepts such as car, cdr, cons cell and list.  */)
-     (list)
-     register Lisp_Object list;
-{
-  return CAR (list);
-}
-
-DEFUN ("car-safe", Fcar_safe, Scar_safe, 1, 1, 0,
-       doc: /* Return the car of OBJECT if it is a cons cell, or else nil.  */)
-     (object)
-     Lisp_Object object;
-{
-  return CAR_SAFE (object);
-}
-
-DEFUN ("cdr", Fcdr, Scdr, 1, 1, 0,
-       doc: /* Return the cdr of LIST.  If arg is nil, return nil.
-Error if arg is not nil and not a cons cell.  See also `cdr-safe'.
-
-See Info node `(elisp)Cons Cells' for a discussion of related basic
-Lisp concepts such as cdr, car, cons cell and list.  */)
-     (list)
-     register Lisp_Object list;
-{
-  return CDR (list);
-}
-
-DEFUN ("cdr-safe", Fcdr_safe, Scdr_safe, 1, 1, 0,
-       doc: /* Return the cdr of OBJECT if it is a cons cell, or else nil.  */)
-     (object)
-     Lisp_Object object;
-{
-  return CDR_SAFE (object);
-}
-
 DEFUN ("setcar", Fsetcar, Ssetcar, 2, 2, 0,
        doc: /* Set the car of CELL to be NEWCAR.  Returns NEWCAR.  */)
      (cell, newcar)
@@ -2933,6 +2890,57 @@
   return make_number (order);
 }
 
+#if defined (__GNUC__) && defined (__OPTIMIZE__) && !defined(__OPTIMIZE_SIZE__)
+/* If we're optimizing, these are macros, and they will conflict with
+   the definitions below.  */
+#undef Fcar
+#undef Fcar_safe
+#undef Fcdr
+#undef Fcdr_safe
+#endif
+
+\f
+/* Extract and set components of lists */
+
+DEFUN ("car", Fcar, Scar, 1, 1, 0,
+       doc: /* Return the car of LIST.  If arg is nil, return nil.
+Error if arg is not nil and not a cons cell.  See also `car-safe'.
+
+See Info node `(elisp)Cons Cells' for a discussion of related basic
+Lisp concepts such as car, cdr, cons cell and list.  */)
+     (list)
+     register Lisp_Object list;
+{
+  return CAR (list);
+}
+
+DEFUN ("car-safe", Fcar_safe, Scar_safe, 1, 1, 0,
+       doc: /* Return the car of OBJECT if it is a cons cell, or else nil.  */)
+     (object)
+     Lisp_Object object;
+{
+  return CAR_SAFE (object);
+}
+
+DEFUN ("cdr", Fcdr, Scdr, 1, 1, 0,
+       doc: /* Return the cdr of LIST.  If arg is nil, return nil.
+Error if arg is not nil and not a cons cell.  See also `cdr-safe'.
+
+See Info node `(elisp)Cons Cells' for a discussion of related basic
+Lisp concepts such as cdr, car, cons cell and list.  */)
+     (list)
+     register Lisp_Object list;
+{
+  return CDR (list);
+}
+
+DEFUN ("cdr-safe", Fcdr_safe, Scdr_safe, 1, 1, 0,
+       doc: /* Return the cdr of OBJECT if it is a cons cell, or else nil.  */)
+     (object)
+     Lisp_Object object;
+{
+  return CDR_SAFE (object);
+}
 
 \f
 void
Index: lisp.h
===================================================================
RCS file: /sources/emacs/emacs/src/lisp.h,v
retrieving revision 1.577
diff -u -r1.577 lisp.h
--- lisp.h	8 Jun 2007 19:56:24 -0000	1.577
+++ lisp.h	20 Jun 2007 14:11:00 -0000
@@ -2206,10 +2206,39 @@
 EXFUN (Finteger_or_floatp, 1);
 EXFUN (Finteger_or_float_or_marker_p, 1);
 
+/* If using GNU and optimizing for speed,
+   inline the most common cons operations.  */
+#if defined (__GNUC__) && defined (__OPTIMIZE__) && !defined(__OPTIMIZE_SIZE__)
+
+#define CONSFN(name) \
+  static __inline__ Lisp_Object \
+  _F ## name (obj) \
+       Lisp_Object obj; \
+  { \
+    return name (obj); \
+  }
+
+CONSFN (CAR)
+CONSFN (CAR_SAFE)
+CONSFN (CDR)
+CONSFN (CDR_SAFE)
+
+#define Fcar(c) _FCAR (c)
+#define Fcar_safe(c) _FCAR_SAFE (c)
+#define Fcdr(c) _FCDR (c)
+#define Fcdr_safe(c) _FCDR_SAFE (c)
+
+#undef CONSFN
+
+#else
+
 EXFUN (Fcar, 1);
 EXFUN (Fcar_safe, 1);
 EXFUN (Fcdr, 1);
 EXFUN (Fcdr_safe, 1);
+
+#endif /* __GNUC__ && __OPTIMIZE__ && !__OPTIMIZE_SIZE__ */
+
 EXFUN (Fsetcar, 2);
 EXFUN (Fsetcdr, 2);
 EXFUN (Fboundp, 1);

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Redundant type checking in window.c and w32menu.c

[Jason Rumney]

Dmitry Antipov wrote:

> +#define Fcar(c) _FCAR (c)
> +#define Fcar_safe(c) _FCAR_SAFE (c)
> +#define Fcdr(c) _FCDR (c)
> +#define Fcdr_safe(c) _FCDR_SAFE (c)

How does lisp code then call these C macros?

Re: Redundant type checking in window.c and w32menu.c

[dmantipov]

Jason Rumney wrote:

> Dmitry Antipov wrote:
>
>> +#define Fcar(c) _FCAR (c)
>> +#define Fcar_safe(c) _FCAR_SAFE (c)
>> +#define Fcdr(c) _FCDR (c)
>> +#define Fcdr_safe(c) _FCDR_SAFE (c)
>
> How does lisp code then call these C macros?

It will call original functions which are preserved in data.c. Lisp code calls them via pointers, so we definitely need to preserve an addressable versions.

Dmitry

Re: Redundant type checking in window.c and w32menu.c

[Tom Tromey]

>>>>> "Dmitry" == Dmitry Antipov <dmantipov@yandex.ru> writes:

Dmitry> +#define CONSFN(name) \
Dmitry> +  static __inline__ Lisp_Object \
Dmitry> +  _F ## name (obj) \
Dmitry> +       Lisp_Object obj; \
Dmitry> +  { \
Dmitry> +    return name (obj); \
Dmitry> +  }

IMO, simply spelling out 4 the definitions here would not be much more
verbose, but it would be clearer and would let _FCAR et al show up in
TAGS.  What do you think?

Tom

Re: Redundant type checking in window.c and w32menu.c

[Jason Rumney]

dmantipov wrote:
> Jason Rumney wrote
>> Dmitry Antipov wrote:
>>
>>     
>>> +#define Fcar(c) _FCAR (c)
>>> +#define Fcar_safe(c) _FCAR_SAFE (c)
>>> +#define Fcdr(c) _FCDR (c)
>>> +#define Fcdr_safe(c) _FCDR_SAFE (c)
>>>       
>> How does lisp code then call these C macros?
>>     
>
> It will call original functions which are preserved in data.c. Lisp code calls them via pointers, so we definitely need to preserve an addressable versions.
>   


Is this optimisation really worth the confusion of having two versions
of these functions in the code?

Re: Redundant type checking in window.c and w32menu.c

[Dmitry Antipov]

Jason Rumney wrote:

> dmantipov wrote:
>> Jason Rumney wrote
>>> Dmitry Antipov wrote:
>>>
>>>     
>>>> +#define Fcar(c) _FCAR (c)
>>>> +#define Fcar_safe(c) _FCAR_SAFE (c)
>>>> +#define Fcdr(c) _FCDR (c)
>>>> +#define Fcdr_safe(c) _FCDR_SAFE (c)
>>>>       
>>> How does lisp code then call these C macros?
>>>     
>> It will call original functions which are preserved in data.c. Lisp code calls them via pointers, so we definitely need to preserve an addressable versions.
>>   
> 
> 
> Is this optimisation really worth the confusion of having two versions
> of these functions in the code?

YMMV :-) - since these functions has the only arg, we shouldn't loose too much
on passing an arguments and performing the call. Probably this also depends
on compiler options passed and even from the compiler's version.

Another interesting thing to try (probably x86 only) is to use
__attribute__((regparm (1))) and see what happens...

Dmitry

Re: Redundant type checking in window.c and w32menu.c

[Ken Raeburn]

On Jun 20, 2007, at 16:46, Jason Rumney wrote:
> Is this optimisation really worth the confusion of having two versions
> of these functions in the code?

We could use a new macro DEFUN_INLINE, like so:

#if CAN_DO_INLINE || defined PROVIDE_ADDRESSABLE_IMPLEMENTATION
DEFUN_INLINE ("car", Fcar, Scar, ....)
   (list)
   Lisp_Object list;
{
   ...;
}
#else
EXFUN(Fcar, 1);
#endif

where:

PROVIDE_ADDRESSABLE_IMPLEMENTATION: Is defined in exactly one Emacs  
source file, say, data.c, before lisp.h is included.

CAN_DO_INLINE: Is true if the compiler, with the current options,  
supports inline functions in some useful form.  (E.g., it could  
expand to test __GNUC__, __OPTIMIZE__, __STDC_VERSION__, etc.)  For  
brevity, we could also define a macro that tests both this property  
and PROVIDE_ADDRESSABLE_IMPLEMENTATION.

DEFUN_INLINE: If inline functions are not supported, expands to the  
same as DEFUN.  Otherwise, tests PROVIDE_ADDRESSABLE_IMPLEMENTATION  
and makes an addressable version if it's defined, and an inline  
version if not.  Depending on the compiler, "inline", "static  
inline", "extern inline", or other keywords may be used.

In older GCC compilers, "extern inline" means "expand inline or  
generate a reference to an external copy defined elsewhere, but don't  
provide an external definition or static copy", while "inline" would  
produce an external definition.  ISO C 99 has a different meaning for  
"extern inline", in fact I think the handling of "extern inline" and  
"inline" may be almost exactly reversed from the old GCC handling,  
and GCC is adapting to the spec but providing some attributes for  
finer control I think.  Using "static inline" sidesteps the issue but  
risks producing multiple static copies of the function if it's not  
expanded inline.  But there are also GCC version number macro tests  
and autoconf feature test cases possible.  This could make defining  
DEFUN_INLINE a little bit complicated, but the use of it should be  
pretty straightforward.

Then, data.o gets an addressable copy of the function for use in  
defining the Lisp symbol, and it can be expanded inline in other  
source files when the compiler supports it.  Debuggers on some older  
platforms may not handle functions defined in header files very well,  
but with inline expansion happening, you're not likely to be able to  
set breakpoints in these functions usefully anyways.

(On a vaguely related note, is it time to start assuming at least  
ANSI C '89 support, like prototypes, in the Emacs sources yet?)

Ken