Re: [PATCH] MIPS: PowerTV: Use fls() carefully where static optimization is required

[Shinya Kuribayashi <skuribay@pobox.com>]

Hi,

On 07/01/2010 07:01 AM, David VomLehn wrote:
> Thanks!  You are correct in your analysis and make a good point that
> clz should be used in interrupt handling. I think, though, that it's
> better to go ahead and supply a full-blown cpu-features-override.h 
> rather than focusing on this one case. This way fls() will be optimized
> to use clz everywhere and any other optimizations that depend on constant
> cpu_has_* values will also be used.

Your choice, either one will be fine :-)

By the way, Malta's clz() and irq_ffs() are very nice, and there are
two followers; MIPSSim and PowerTV.  And now I'm going to make use of
them for emma2rh, too.

I've prepared a consolidation patch like this, but have two concerns:

1) irq_ffs() is used to dispatch IRQs, so we'd like to give preference
   to CONFIG_CPU_xxx over cpu_has_clo_clz, to optimize with CLZ.  It's
   somewhat different for usual fls() and ffs() cases.  Or, 

2) would it be better to check __builtin_constant_p(cpu_has_clo_clz)?

Or, any other good alternatives?


diff --git a/arch/mips/include/asm/irq_cpu.h b/arch/mips/include/asm/irq_cpu.h
index ef6a07c..ad6b416 100644
--- a/arch/mips/include/asm/irq_cpu.h
+++ b/arch/mips/include/asm/irq_cpu.h
@@ -17,4 +17,47 @@ extern void mips_cpu_irq_init(void);
 extern void rm7k_cpu_irq_init(void);
 extern void rm9k_cpu_irq_init(void);
 
+/*
+ * Version of ffs that only looks at bits 12..15.
+ */
+static inline unsigned int irq_ffs(unsigned int pending)
+{
+#if defined(CONFIG_CPU_MIPS32) || defined(CONFIG_CPU_MIPS64)
+	int x;
+
+	__asm__(
+	"	.set	push					\n"
+	"	.set	mips32					\n"
+	"	clz	%0, %1					\n"
+	"	.set	pop					\n"
+	: "=r" (x)
+	: "r" (pending));
+
+	return -x + 31 - CAUSEB_IP;
+#else
+	unsigned int a0 = 7;
+	unsigned int t0;
+
+	t0 = pending & 0xf000;
+	t0 = t0 < 1;
+	t0 = t0 << 2;
+	a0 = a0 - t0;
+	pending = pending << t0;
+
+	t0 = pending & 0xc000;
+	t0 = t0 < 1;
+	t0 = t0 << 1;
+	a0 = a0 - t0;
+	pending = pending << t0;
+
+	t0 = pending & 0x8000;
+	t0 = t0 < 1;
+	/* t0 = t0 << 2; */
+	a0 = a0 - t0;
+	/* pending = pending << t0; */
+
+	return a0;
+#endif
+}
+
 #endif /* _ASM_IRQ_CPU_H */
diff --git a/arch/mips/mipssim/sim_int.c b/arch/mips/mipssim/sim_int.c
index 5c779be..5813e7e 100644
--- a/arch/mips/mipssim/sim_int.c
+++ b/arch/mips/mipssim/sim_int.c
@@ -22,52 +22,6 @@
 #include <asm/mips-boards/simint.h>
 #include <asm/irq_cpu.h>
 
-static inline int clz(unsigned long x)
-{
-	__asm__(
-	"	.set	push					\n"
-	"	.set	mips32					\n"
-	"	clz	%0, %1					\n"
-	"	.set	pop					\n"
-	: "=r" (x)
-	: "r" (x));
-
-	return x;
-}
-
-/*
- * Version of ffs that only looks at bits 12..15.
- */
-static inline unsigned int irq_ffs(unsigned int pending)
-{
-#if defined(CONFIG_CPU_MIPS32) || defined(CONFIG_CPU_MIPS64)
-	return -clz(pending) + 31 - CAUSEB_IP;
-#else
-	unsigned int a0 = 7;
-	unsigned int t0;
-
-	t0 = s0 & 0xf000;
-	t0 = t0 < 1;
-	t0 = t0 << 2;
-	a0 = a0 - t0;
-	s0 = s0 << t0;
-
-	t0 = s0 & 0xc000;
-	t0 = t0 < 1;
-	t0 = t0 << 1;
-	a0 = a0 - t0;
-	s0 = s0 << t0;
-
-	t0 = s0 & 0x8000;
-	t0 = t0 < 1;
-	/* t0 = t0 << 2; */
-	a0 = a0 - t0;
-	/* s0 = s0 << t0; */
-
-	return a0;
-#endif
-}
-
 asmlinkage void plat_irq_dispatch(void)
 {
 	unsigned int pending = read_c0_cause() & read_c0_status() & ST0_IM;
diff --git a/arch/mips/mti-malta/malta-int.c b/arch/mips/mti-malta/malta-int.c
index 15949b0..656cecf 100644
--- a/arch/mips/mti-malta/malta-int.c
+++ b/arch/mips/mti-malta/malta-int.c
@@ -197,52 +197,6 @@ static void corehi_irqdispatch(void)
 	die("CoreHi interrupt", regs);
 }
 
-static inline int clz(unsigned long x)
-{
-	__asm__(
-	"	.set	push					\n"
-	"	.set	mips32					\n"
-	"	clz	%0, %1					\n"
-	"	.set	pop					\n"
-	: "=r" (x)
-	: "r" (x));
-
-	return x;
-}
-
-/*
- * Version of ffs that only looks at bits 12..15.
- */
-static inline unsigned int irq_ffs(unsigned int pending)
-{
-#if defined(CONFIG_CPU_MIPS32) || defined(CONFIG_CPU_MIPS64)
-	return -clz(pending) + 31 - CAUSEB_IP;
-#else
-	unsigned int a0 = 7;
-	unsigned int t0;
-
-	t0 = pending & 0xf000;
-	t0 = t0 < 1;
-	t0 = t0 << 2;
-	a0 = a0 - t0;
-	pending = pending << t0;
-
-	t0 = pending & 0xc000;
-	t0 = t0 < 1;
-	t0 = t0 << 1;
-	a0 = a0 - t0;
-	pending = pending << t0;
-
-	t0 = pending & 0x8000;
-	t0 = t0 < 1;
-	/* t0 = t0 << 2; */
-	a0 = a0 - t0;
-	/* pending = pending << t0; */
-
-	return a0;
-#endif
-}
-
 /*
  * IRQs on the Malta board look basically (barring software IRQs which we
  * don't use at all and all external interrupt sources are combined together
diff --git a/arch/mips/powertv/asic/asic_int.c b/arch/mips/powertv/asic/asic_int.c
index e3c08a2..06602e7 100644
--- a/arch/mips/powertv/asic/asic_int.c
+++ b/arch/mips/powertv/asic/asic_int.c
@@ -68,27 +68,6 @@ static void asic_irqdispatch(void)
 	do_IRQ(irq);
 }
 
-static inline int clz(unsigned long x)
-{
-	__asm__(
-	"	.set	push					\n"
-	"	.set	mips32					\n"
-	"	clz	%0, %1					\n"
-	"	.set	pop					\n"
-	: "=r" (x)
-	: "r" (x));
-
-	return x;
-}
-
-/*
- * Version of ffs that only looks at bits 12..15.
- */
-static inline unsigned int irq_ffs(unsigned int pending)
-{
-	return -clz(pending) + 31 - CAUSEB_IP;
-}
-
 /*
  * TODO: check how it works under EIC mode.
  */