RE: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
+#define __BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
@@ -85,6 +86,8 @@
} \
} while (0)
+#define BUG_ON(x) __BUG_ON(unlikely(x))
+
From my experiments, adding an 'unlikely' at that point isn't
enough for non-trivial conditions - so its presence will
give a false sense the the optimisation is present!
In particular 'if (unlikely(x y))' needs to be
'if (unlikely(x) unlikely(y))' in order to avoid
mispredicted branches when 'x' is false.
Also, as (I think) in some of the generated code quoted,
use of __builtin_expect() with a boolean expression can
force some versions of gcc to generate the integer
value of the expression - rather than just selecting the
branch instructions that statically predict the
normal code path.
Sometimes I've also also had to add an asm() statement
that generates no code in order to actually force a
forwards branch (so it has something to place at the
target).
(I've been counting clocks )
David
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
David Laight david.lai...@aculab.com writes: Also, as (I think) in some of the generated code quoted, use of __builtin_expect() with a boolean expression can force some versions of gcc to generate the integer value of the expression That's more likely a side effect of the definition of likely/unlikely: they expand to !!(x). Andreas. -- Andreas Schwab, sch...@redhat.com GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84 5EC7 45C6 250E 6F00 984E And now for something completely different. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
On 2011年01月28日 18:14, Andreas Schwab Wrote: David Laightdavid.lai...@aculab.com writes: Also, as (I think) in some of the generated code quoted, use of __builtin_expect() with a boolean expression can force some versions of gcc to generate the integer value of the expression That's more likely a side effect of the definition of likely/unlikely: they expand to !!(x). It seems whether or not using unlikely() inside arch implemented BUG_ON() is arch dependent. Maybe a reasonable method to use BUG_ON() is, 1) do not explicitly use unlikely() when using macro BUG_ON(). 2) whether or not using unlikely() inside BUG_ON(), it depends on the implementation of BUG_ON(), including arch implementation. So from current feed back, doing unlikely() optimization here doesn't make anything better. Thanks for all of your feed back :-) -- Coly Li ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
Why not also CC the PPC maintainers as well? I am not certain, but I
think they may be reached at:
linuxppc-dev@lists.ozlabs.org
On 01/27/2011 04:12 AM, Coly Li wrote:
Current BUG_ON() arch/powerpc/include/asm/bug.h does not use unlikely(),
in order to get better branch predict result, source code may have to call
BUG_ON() with unlikely() explicitly. This is not a suggested method
to use BUG_ON().
This patch adds unlikely() inside BUG_ON implementation on PPC
code, callers can use BUG_ON without explicit unlikely() now.
I don't have any PPC hardware to compile and test this fix, any feedback
of this patch is welcome.
Signed-off-by: Coly Libosong...@taobao.com
Cc: Jeremy Fitzhardingejer...@goop.org
Cc: David Daneydda...@caviumnetworks.com
Cc: Wang Congxiyou.wangc...@gmail.com
Cc: Yong Zhangyong.zha...@gmail.com
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 065c590..10889a6 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -2,6 +2,7 @@
#define _ASM_POWERPC_BUG_H
#ifdef __KERNEL__
+#includelinux/compiler.h
#includeasm/asm-compat.h
/*
@@ -71,7 +72,7 @@
unreachable(); \
} while (0)
-#define BUG_ON(x) do { \
+#define __BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
@@ -85,6 +86,8 @@
} \
} while (0)
+#define BUG_ON(x) __BUG_ON(unlikely(x))
+
This is the same type of frobbing you were trying to do to MIPS.
I will let the powerpc maintainers weigh in on it, but my opinion is
that, as with MIPS, BUG_ON() is expanded to a single machine
instruction, and this unlikely() business will not change the generated
code in any useful way. It is thus gratuitous code churn and
complexification.
David Daney
#define __WARN_TAINT(taint) do { \
__asm__ __volatile__( \
1:twi 31,0,0\n \
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
On Thu, 27 Jan 2011 09:57:39 -0800
David Daney dda...@caviumnetworks.com wrote:
On 01/27/2011 04:12 AM, Coly Li wrote:
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 065c590..10889a6 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -2,6 +2,7 @@
#define _ASM_POWERPC_BUG_H
#ifdef __KERNEL__
+#includelinux/compiler.h
#includeasm/asm-compat.h
/*
@@ -71,7 +72,7 @@
unreachable(); \
} while (0)
-#define BUG_ON(x) do { \
+#define __BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
@@ -85,6 +86,8 @@
} \
} while (0)
+#define BUG_ON(x) __BUG_ON(unlikely(x))
+
This is the same type of frobbing you were trying to do to MIPS.
I will let the powerpc maintainers weigh in on it, but my opinion is
that, as with MIPS, BUG_ON() is expanded to a single machine
instruction, and this unlikely() business will not change the generated
code in any useful way. It is thus gratuitous code churn and
complexification.
What about just doing this:
#define BUG() __builtin_trap()
#define BUG_ON(x) do { \
if (x) \
BUG(); \
} while (0)
GCC should produce better code than forcing it into twnei. A simple
BUG_ON(x != y) currently generates something like this (GCC 4.3):
xor r0,r11,r0
addic r10,r0,-1
subfe r9,r10,r0
twnei r9,0
Or this (GCC 4.5):
xor r0,r11,r0
cntlzw r0,r0
srwir0,r0,5
xorir0,r0,1
twnei r0,0
Instead of:
twner0,r11
And if GCC doesn't treat code paths that lead to __builtin_trap() as
unlikely (which could make a difference with complex expressions,
even with a conditional trap instruction), that's something that could
and should be fixed in GCC.
The downside is that GCC says, The mechanism used may vary from
release to release so you should not rely on any particular
implementation. However, some architectures (sparc, m68k, ia64)
already use __builtin_trap() for this, it seems unlikely that future GCC
versions would switch away from using the trap instruction[1], and there
doesn't seem to be a better-defined way to make GCC generate trap
instructions intelligently.
-Scott
[1] A more likely possibility is that an older compiler just generates a
call to abort() or similar, and later versions implemented trap -- need
to check what the oldest supported GCC does.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 2/7] PowerPC: add unlikely() to BUG_ON()
On 01/27/2011 12:04 PM, Scott Wood wrote:
On Thu, 27 Jan 2011 09:57:39 -0800
David Daneydda...@caviumnetworks.com wrote:
On 01/27/2011 04:12 AM, Coly Li wrote:
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 065c590..10889a6 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -2,6 +2,7 @@
#define _ASM_POWERPC_BUG_H
#ifdef __KERNEL__
+#includelinux/compiler.h
#includeasm/asm-compat.h
/*
@@ -71,7 +72,7 @@
unreachable(); \
} while (0)
-#define BUG_ON(x) do { \
+#define __BUG_ON(x) do { \
if (__builtin_constant_p(x)) { \
if (x) \
BUG(); \
@@ -85,6 +86,8 @@
} \
} while (0)
+#define BUG_ON(x) __BUG_ON(unlikely(x))
+
This is the same type of frobbing you were trying to do to MIPS.
I will let the powerpc maintainers weigh in on it, but my opinion is
that, as with MIPS, BUG_ON() is expanded to a single machine
instruction, and this unlikely() business will not change the generated
code in any useful way. It is thus gratuitous code churn and
complexification.
What about just doing this:
#define BUG() __builtin_trap()
#define BUG_ON(x) do { \
if (x) \
BUG(); \
} while (0)
GCC should produce better code than forcing it into twnei. A simple
BUG_ON(x != y) currently generates something like this (GCC 4.3):
xor r0,r11,r0
addic r10,r0,-1
subfe r9,r10,r0
twnei r9,0
Or this (GCC 4.5):
xor r0,r11,r0
cntlzw r0,r0
srwir0,r0,5
xorir0,r0,1
twnei r0,0
Instead of:
twner0,r11
And if GCC doesn't treat code paths that lead to __builtin_trap() as
unlikely (which could make a difference with complex expressions,
even with a conditional trap instruction), that's something that could
and should be fixed in GCC.
The downside is that GCC says, The mechanism used may vary from
release to release so you should not rely on any particular
implementation. However, some architectures (sparc, m68k, ia64)
already use __builtin_trap() for this, it seems unlikely that future GCC
versions would switch away from using the trap instruction[1], and there
doesn't seem to be a better-defined way to make GCC generate trap
instructions intelligently.
This is good in theory, however powerpc has this _EMIT_BUG_ENTRY
business that wouldn't work with __builtin_trap().
David Daney
-Scott
[1] A more likely possibility is that an older compiler just generates a
call to abort() or similar, and later versions implemented trap -- need
to check what the oldest supported GCC does.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
