Re: [PATCH] powerpc: Make BUG_ON & WARN_ON play nice with compile-time optimisations

Re: [PATCH] powerpc: Make BUG_ON & WARN_ON play nice with compile-time optimisations

[Michael Ellerman]

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

On Tue, 7 Feb 2006 16:22, Michael Ellerman wrote:
> Currently if you do BUG_ON(0) you'll still get a trap instruction in your
> object, although it'll never trigger. That's ok, but a bit ugly, it'd be
> nice if the compiler could completely eliminate any trace of the BUG_ON.
>
> So update the BUG_ON & WARN_ON macros to make this possible. From the
> comment in the patch:
>
>  The if statement in BUG_ON and WARN_ON gives the compiler a chance to do
>  compile-time optimisation and possibly elide the entire block. The check
>  for !__builtin_constant(x) has the oppposite effect, if we must do the
>  test at runtime then we avoid a spurious compare and branch by ensuring
>  the if condition is always true.
>
> I've confirmed it works in both cases, if the condition is false at compile
> time we get no code emitted for the BUG statement. If the condition needs
> to be evaluated at runtime we get the same code we used to, ie. only one
> test in the trap instruction.

Turns out this doesn't always do what we want, depending on what the condition 
for the BUG_ON() is. Specifically the __builtin_constant_p() sometimes fails 
to recognise that the condition will be constant, and so we end up with:

 558:   38 00 00 00     li      r0,0
 55c:   0b 00 00 00     tdnei   r0,0

Which is harmless but not really good enough. When gcc gets fixed 
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26724) we can think about this 
again.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[RFC/PATCH] powerpc: Make BUG_ON & WARN_ON play nice with compile-time optimisations

[Michael Ellerman]

Although we could do this. It relys on firmware_has_feature() being a macro,
but perhaps that's ok. This isn't exactly ideal, as it encourages us to use
macros where we could otherwise use static inlines, but perhaps it's ok.

Given this:

void test(void)
{
       constant_false();
       BUG_ON(firmware_has_feature(FW_FEATURE_ISERIES));

       constant_true();
       BUG_ON(!firmware_has_feature(FW_FEATURE_ISERIES));

       non_constant();
       BUG_ON(firmware_has_feature(FW_FEATURE_SPLPAR));

       constant_false();
       WARN_ON(firmware_has_feature(FW_FEATURE_ISERIES));

       constant_true();
       WARN_ON(!firmware_has_feature(FW_FEATURE_ISERIES));

       non_constant();
       WARN_ON(firmware_has_feature(FW_FEATURE_SPLPAR));
}

I get assembly like so, which looks good to me:

        bl .constant_false
        bl .constant_true
        1:      twi 31,0,0
.section __bug_table,"a"
        .llong  1b,400,.LC12,__func__.21353
.previous
        bl .non_constant
        ld 9,.LC13@toc(2)
        ld 0,0(9)
        rldicl 0,0,44,63
        1:      tdnei   0,0
.section __bug_table,"a"
        .llong  1b,403,.LC12,__func__.21353
.previous
        bl .constant_false
        bl .constant_true
        1:      twi 31,0,0
.section __bug_table,"a"
        .llong  1b,16777625,.LC12,__func__.21353
.previous
        bl .non_constant
        ld 9,.LC13@toc(2)
        ld 0,0(9)
        rldicl 0,0,44,63
        1:      tdnei   0,0
.section __bug_table,"a"
        .llong  1b,16777628,.LC12,__func__.21353
.previous



 include/asm-powerpc/bug.h      |   24 ++++++++++++++++++++++--
 include/asm-powerpc/firmware.h |    8 +++-----
 2 files changed, 25 insertions(+), 7 deletions(-)

Index: to-merge/include/asm-powerpc/bug.h
===================================================================
--- to-merge.orig/include/asm-powerpc/bug.h
+++ to-merge/include/asm-powerpc/bug.h
@@ -40,17 +40,36 @@ struct bug_entry *find_bug(unsigned long
 } while (0)
 
 #define BUG_ON(x) do {						\
-	__asm__ __volatile__(					\
+	if (__builtin_constant_p(x)) {				\
+		if (x)						\
+			BUG();					\
+	} else {						\
+		__asm__ __volatile__(				\
 		"1:	"PPC_TLNEI"	%0,0\n"			\
 		".section __bug_table,\"a\"\n"			\
 		"\t"PPC_LONG"	1b,%1,%2,%3\n"		\
 		".previous"					\
 		: : "r" ((long)(x)), "i" (__LINE__),		\
 		    "i" (__FILE__), "i" (__FUNCTION__));	\
+	}							\
 } while (0)
 
-#define WARN_ON(x) do {						\
+#define WARN() do {						\
 	__asm__ __volatile__(					\
+		"1:	twi 31,0,0\n"				\
+		".section __bug_table,\"a\"\n"			\
+		"\t"PPC_LONG"	1b,%0,%1,%2\n"			\
+		".previous"					\
+		: : "i" (__LINE__ + BUG_WARNING_TRAP),		\
+		    "i" (__FILE__), "i" (__FUNCTION__));	\
+} while (0)
+
+#define WARN_ON(x) do {						\
+	if (__builtin_constant_p(x)) {				\
+		if (x)						\
+			WARN();					\
+	} else {						\
+		__asm__ __volatile__(				\
 		"1:	"PPC_TLNEI"	%0,0\n"			\
 		".section __bug_table,\"a\"\n"			\
 		"\t"PPC_LONG"	1b,%1,%2,%3\n"		\
@@ -58,6 +77,7 @@ struct bug_entry *find_bug(unsigned long
 		: : "r" ((long)(x)),				\
 		    "i" (__LINE__ + BUG_WARNING_TRAP),		\
 		    "i" (__FILE__), "i" (__FUNCTION__));	\
+	}							\
 } while (0)
 
 #define HAVE_ARCH_BUG
Index: to-merge/include/asm-powerpc/firmware.h
===================================================================
--- to-merge.orig/include/asm-powerpc/firmware.h
+++ to-merge/include/asm-powerpc/firmware.h
@@ -83,11 +83,9 @@ enum {
  */
 extern unsigned long	ppc64_firmware_features;
 
-static inline unsigned long firmware_has_feature(unsigned long feature)
-{
-	return (FW_FEATURE_ALWAYS & feature) ||
-		(FW_FEATURE_POSSIBLE & ppc64_firmware_features & feature);
-}
+#define firmware_has_feature(feature)					\
+	((FW_FEATURE_ALWAYS & (feature)) ||				\
+		(FW_FEATURE_POSSIBLE & ppc64_firmware_features & (feature)))
 
 extern void system_reset_fwnmi(void);
 extern void machine_check_fwnmi(void);

Re: [RFC/PATCH] powerpc: Make BUG_ON & WARN_ON play nice with compile-time optimisations

[Stephen Rothwell]

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

On Tue, 21 Mar 2006 16:51:29 +1100 Michael Ellerman <michael@ellerman.id.au> wrote:
>
> Although we could do this. It relys on firmware_has_feature() being a macro,
> but perhaps that's ok. This isn't exactly ideal, as it encourages us to use
> macros where we could otherwise use static inlines, but perhaps it's ok.

In this case the macros are small and the overall effect looks good.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 191 bytes --]