Re: [PATCH] powerpc: optimise WARN_ON()

On Mon, Aug 19, 2019 at 07:40:42AM +0200, Christophe Leroy wrote:

Le 18/08/2019 à 14:01, Segher Boessenkool a écrit :

quoted

On Sat, Aug 17, 2019 at 09:04:42AM +0000, Christophe Leroy wrote:

quoted

Unlike BUG_ON(x), WARN_ON(x) uses !!(x) as the trigger
of the t(d/w)nei instruction instead of using directly the
value of x.

This leads to GCC adding unnecessary pair of addic/subfe.

And it has to, it is passed as an "r" to an asm, GCC has to put the "!!"
value into a register.

quoted

By using (x) instead of !!(x) like BUG_ON() does, the additional
instructions go away:

But is it correct?  What happens if you pass an int to WARN_ON, on a
64-bit kernel?

On a 64-bit kernel, an int is still in a 64-bit register, so there would 
be no problem with tdnei, would it ? an int 0 is the same as an long 0, 
right ?

The top 32 bits of a 64-bit register holding an int are undefined.  Take
as example

  int x = 42;
  x = ~x;

which may put ffff_ffff_ffff_ffd5 into the reg, not 0000_0000_ffff_ffd5
as you might expect or want.  For tw instructions this makes no difference
(they only look at the low 32 bits anyway); for td insns, it does.

It is on 32-bit kernel that I see a problem, if one passes a long long 
to WARN_ON(), the forced cast to long will just drop the upper size of 
it. So as of today, BUG_ON() is buggy for that.

Sure, it isn't defined what types you can pass to that macro.  Another
thing that makes inline functions much saner to use.

quoted

(You might want to have 64-bit generate either tw or td.  But, with
your __builtin_trap patch, all that will be automatic).

Yes I'll discard this patch and focus on the __builtin_trap() one which 
should solve most issues.

But see my comment there about the compiler knowing all code after an
unconditional trap is dead.


Segher