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

[Ralf Baechle <ralf@linux-mips.org>]

On Mon, Jul 05, 2010 at 09:33:36AM +0900, Shinya Kuribayashi wrote:

> On 7/4/2010 2:03 AM, Maciej W. Rozycki wrote:
> >  Malta also supports a couple of MIPS IV processors too, so please be 
> > careful about such assumptions.
> 
> Ah, that's the answer I'm looking for, thanks!  So current irq_ffs()
> form (clz() is enabled only when CONFIG_CPU_MIPS32/64 is selected) is
> well-suited for Malta platform, and it seems better to leave them as
> they are.  I'll drop the patch from my list.
> 
> >> +	if (__builtin_constant_p(cpu_has_clo_clz) && cpu_has_clo_clz) {
> >> +		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;
> >> +	}
> > 
> >  Hmm, ".set mips32" looks dodgy here.  For pre-MIPS32/64 platforms this 
> > code should never make it to the assembler and if it did, then a 
> > build-time error is better than a run-time problem.

For pedantic accuracy - the IDT RC32364 introduced CLO and CLZ; in an act
of uglyness the RC64574 then inherited these two instructions but did not
add. though it was 64-bit not DCLO and DCLZ; the NEC VR5500 has the full
complement of CLO, CLZ, DCLO and DCLZ.

> I see, cpu_has_clo_clz doesn't work well for platforms such as Malta.
> Malta can support several ISAs at a time, which is very valuable, but
> hard to be optimized :-)

While MIPS IV CPU cards for the Malta are available hardly anybody is using
on of those cards.  Thus cpu_has_clo_clz defaults to cpu_has_mips_r and
ideally and platform should see cpu_has_mips_r to a constant to allow best
possible optimization.  Malta doesn't ...

> >  It might be simpler just to use __builtin_ffs() for this variant though.  
> > Inline assembly is better avoided unless absolutely required.  Not even 
> > mentioning readability.
> 
> Hm.  It might be simpler, but it's not the purpose of irq_ffs(), IMHO.

Indeed.

  Ralf