Re: [PATCH] likely cleanup: remove unlikely for kfree(NULL)

[Bart Hartgers <bart@etpmod.phys.tue.nl>]

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

Bart Hartgers wrote:
> Jörn Engel wrote:
>> On Wed, 26 April 2006 13:03:34 +0200, Arjan van de Ven wrote:
>>> On Wed, 2006-04-26 at 13:57 +0300, Pekka J Enberg wrote:
>>>> On Wed, 26 April 2006 10:27:18 +0200, Arjan van de Ven wrote:
>>>>>>> what I would like is kfree to become an inline wrapper that does the
>>>>>>> null check inline, that way gcc can optimize it out (and it will in 4.1
>>>>>>> with the VRP pass) if gcc can prove it's not NULL.
>>>> On Wed, 2006-04-26 at 12:05 +0200, Jörn Engel wrote:
>>>>>> How well can gcc optimize this case? 
>>>> On Wed, 26 Apr 2006, Arjan van de Ven wrote:
>>>>> if you deref'd the pointer it'll optimize it away (assuming a new enough
>>>>> gcc, like 4.1)
>>>> Here are the numbers for allyesconfig on my setup.
>>>>
>>>> 				Pekka
>>>>
>>>> gcc version 3.4.5 (Gentoo 3.4.5-r1, ssp-3.4.5-1.0, pie-8.7.9)
>>> this is an ancient gcc without VRP so yeah the growth is expected ;)
>> In other words, we shouldn't do this as long as most users don't have
>> gcc 4.1 or higher installed.  So this is somewhat pointless at the
>> moment.
>>
>> Still, if you could respin this with gcc 4.1 and post the numbers,
>> Pekka, that would be quite interesting.
>>
>> Jörn
>>
> 
> What about this:
> 
> static inline void my_kfree( void *ptr )
> {
>         if (__builtin_constant_p(ptr!=NULL)) {
>                 if (ptr!=NULL)
>                         my_fast_free(ptr); /* skips NULL check */
>         } else {
>                 my_checking_free(ptr); /* does a NULL check */
>         }
> }
> 
> That would skip the free when ptr is known to be NULL, and skip the
> equal to NULL check if it is known to be not NULL, and do what happened
> before otherwise. In other words, it is never worse than what we have now.
> 
> Attached is a small testcase in C and the resulting assembly. Note that
> my compiler doesn't catch the "not equal to zero" case, but 4.1 is
> supposed to do this.
> 
> Groeten,
> Bart
> 

Sorry about replying to my own mail, but I discovered that at least "gcc
(GCC) 4.1.0 (SUSE Linux)" does not seem to combine the
delete-null-pointer optimisation with the builtin_constant test. The
compiler is happy to eliminate ptr==NULL tests, but does not consider
the expression (ptr==NULL) constant! I managed to hack around this.

See the attached code, and:

bart@gum15:~> gcc -DCASE_A -m32 -O3 -S -o testje-a.S testje.c
bart@gum15:~> gcc -DCASE_B -m32 -O3 -S -o testje-b.S testje.c
bart@gum15:~> diff -u testje-a.S testje-b.S
--- testje-a.S  2006-04-26 14:57:50.000000000 +0200
+++ testje-b.S  2006-04-26 14:57:53.000000000 +0200
@@ -16,7 +16,7 @@
        addl    $4, %esp
        popl    %ebx
        popl    %ebp
-       jmp     my_fast_free
+       jmp     my_slow_free
        .size   test, .-test
        .ident  "GCC: (GNU) 4.1.0 (SUSE Linux)"
        .section        .note.GNU-stack,"",@progbits

Anyway, CASE_A produces optimal code for gcc 4.1, and gcc 4.0 produces
identical code in both cases.

Groeten,
Bart
-- 
Bart Hartgers - TUE Eindhoven - http://plasimo.phys.tue.nl/bart/contact/

[-- Attachment #2: testje.c --]
[-- Type: text/x-csrc, Size: 501 bytes --]

#include <stddef.h>

extern void my_fast_free(void *);
extern void my_checking_free(void *);

static inline void my_kfree( void *ptr )
{
#ifdef CASE_A
	register int is_null = 0;
	if (ptr == NULL)
		is_null = 1;
	if (__builtin_constant_p(is_null)) {
#else /* CASE_B */
	if (__builtin_constant_p(ptr==NULL)) {
#endif
		if (ptr != NULL)
			my_fast_free(ptr);
	} else {
		my_slow_free(ptr);
	}
}

void test( int *bla )
{
	char *hello = NULL;
	my_kfree(hello);
	my_kfree(bla);
	*bla = 1;
	my_kfree(bla);
}

[-- Attachment #3: testje-a.S --]
[-- Type: text/plain, Size: 387 bytes --]

	.file	"testje.c"
	.text
	.p2align 4,,15
.globl test
	.type	test, @function
test:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	subl	$4, %esp
	movl	8(%ebp), %ebx
	movl	%ebx, (%esp)
	call	my_slow_free
	movl	$1, (%ebx)
	movl	%ebx, 8(%ebp)
	addl	$4, %esp
	popl	%ebx
	popl	%ebp
	jmp	my_fast_free
	.size	test, .-test
	.ident	"GCC: (GNU) 4.1.0 (SUSE Linux)"
	.section	.note.GNU-stack,"",@progbits