RE: [PATCH] x86: bitops: fix build regression

2020-05-10 Thread hpa
On May 10, 2020 4:59:17 AM PDT, David Laight wrote: >From: Peter Anvin >> Sent: 08 May 2020 18:32 >> On 2020-05-08 10:21, Nick Desaulniers wrote: >> >> >> >> One last suggestion. Add the "b" modifier to the mask operand: >"orb >> >> %b1, %0". That forces the compiler to use the 8-bit register

RE: [PATCH] x86: bitops: fix build regression

2020-05-10 Thread David Laight
From: Peter Anvin > Sent: 08 May 2020 18:32 > On 2020-05-08 10:21, Nick Desaulniers wrote: > >> > >> One last suggestion. Add the "b" modifier to the mask operand: "orb > >> %b1, %0". That forces the compiler to use the 8-bit register name > >> instead of trying to deduce the width from the

Re: [PATCH] x86: bitops: fix build regression

2020-05-08 Thread H. Peter Anvin
On 2020-05-08 10:21, Nick Desaulniers wrote: >> >> One last suggestion. Add the "b" modifier to the mask operand: "orb >> %b1, %0". That forces the compiler to use the 8-bit register name >> instead of trying to deduce the width from the input. > > Ah right: >

Re: [PATCH] x86: bitops: fix build regression

2020-05-08 Thread Nick Desaulniers
On Thu, May 7, 2020 at 6:57 PM Brian Gerst wrote: > > On Thu, May 7, 2020 at 6:29 PM Nick Desaulniers > wrote: > > > > On Thu, May 7, 2020 at 12:19 PM Nick Desaulniers > > wrote: > > > > > > On Thu, May 7, 2020 at 7:00 AM Brian Gerst wrote: > > > > > > > > This change will make sparse happy

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Brian Gerst
On Thu, May 7, 2020 at 6:29 PM Nick Desaulniers wrote: > > On Thu, May 7, 2020 at 12:19 PM Nick Desaulniers > wrote: > > > > On Thu, May 7, 2020 at 7:00 AM Brian Gerst wrote: > > > > > > This change will make sparse happy and allow these cleanups: > > > #define CONST_MASK(nr)

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Nick Desaulniers
On Thu, May 7, 2020 at 12:19 PM Nick Desaulniers wrote: > > On Thu, May 7, 2020 at 7:00 AM Brian Gerst wrote: > > > > This change will make sparse happy and allow these cleanups: > > #define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) > > yep, this is more elegant, IMO. Will send a v3

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 7, 2020 8:09:35 AM PDT, David Laight wrote: >From: Brian Gerst >> Sent: 07 May 2020 14:32 >... >> I think the bug this worked around was that the compiler didn't >detect >> that CONST_MASK(nr) was also constant and doesn't need to be put into >> a register. The question is does that bug

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 7, 2020 6:32:24 AM PDT, Brian Gerst wrote: >On Thu, May 7, 2020 at 3:02 AM wrote: >> >> On May 6, 2020 11:18:09 PM PDT, Brian Gerst >wrote: >> >On Tue, May 5, 2020 at 1:47 PM Nick Desaulniers >> > wrote: >> >> >> >> From: Sedat Dilek >> >> >> >> It turns out that if your config tickles

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 7, 2020 4:34:22 AM PDT, Peter Zijlstra wrote: >On Tue, May 05, 2020 at 11:07:24AM -0700, h...@zytor.com wrote: >> On May 5, 2020 10:44:22 AM PDT, Nick Desaulniers > wrote: > >> >@@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long >*addr) >> >if (__builtin_constant_p(nr)) {

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Nick Desaulniers
On Wed, May 6, 2020 at 11:18 PM Brian Gerst wrote: > > I think a better fix would be to make CONST_MASK() return a u8 value > rather than have to cast on every use. > > Also I question the need for the "q" constraint. It was added in > commit 437a0a54 as a workaround for GCC 3.4.4. Now that the

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Nick Desaulniers
On Thu, May 7, 2020 at 7:00 AM Brian Gerst wrote: > > This change will make sparse happy and allow these cleanups: > #define CONST_MASK(nr) ((u8)1 << ((nr) & 7)) yep, this is more elegant, IMO. Will send a v3 later with this change. Looking at the uses of CONST_MASK, I noticed

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread David Laight
From: Brian Gerst > Sent: 07 May 2020 14:32 ... > I think the bug this worked around was that the compiler didn't detect > that CONST_MASK(nr) was also constant and doesn't need to be put into > a register. The question is does that bug still exist on compiler > versions we care about? Hmmm...

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Brian Gerst
On Thu, May 7, 2020 at 7:38 AM Peter Zijlstra wrote: > > On Tue, May 05, 2020 at 11:07:24AM -0700, h...@zytor.com wrote: > > On May 5, 2020 10:44:22 AM PDT, Nick Desaulniers > > wrote: > > > >@@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > > > if

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Brian Gerst
On Thu, May 7, 2020 at 3:02 AM wrote: > > On May 6, 2020 11:18:09 PM PDT, Brian Gerst wrote: > >On Tue, May 5, 2020 at 1:47 PM Nick Desaulniers > > wrote: > >> > >> From: Sedat Dilek > >> > >> It turns out that if your config tickles __builtin_constant_p via > >> differences in choices to

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Peter Zijlstra
On Tue, May 05, 2020 at 11:07:24AM -0700, h...@zytor.com wrote: > On May 5, 2020 10:44:22 AM PDT, Nick Desaulniers > wrote: > >@@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > > if (__builtin_constant_p(nr)) { > > asm volatile(LOCK_PREFIX "orb %1,%0" > >

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Andy Shevchenko
On Thu, May 7, 2020 at 10:50 AM David Laight wrote: > From: Brian Gerst > > Sent: 07 May 2020 07:18 > > I think a better fix would be to make CONST_MASK() return a u8 value > > rather than have to cast on every use. > > Or assign to a local variable - then it doesn't matter how > the value is

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 7, 2020 1:35:01 AM PDT, David Laight wrote: >From: h...@zytor.com >> Sent: 07 May 2020 08:59 >> On May 7, 2020 12:44:44 AM PDT, David Laight > wrote: >> >From: Brian Gerst >> >> Sent: 07 May 2020 07:18 >> >... >> >> > --- a/arch/x86/include/asm/bitops.h >> >> > +++

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread David Laight
From: h...@zytor.com > Sent: 07 May 2020 08:59 > On May 7, 2020 12:44:44 AM PDT, David Laight wrote: > >From: Brian Gerst > >> Sent: 07 May 2020 07:18 > >... > >> > --- a/arch/x86/include/asm/bitops.h > >> > +++ b/arch/x86/include/asm/bitops.h > >> > @@ -54,7 +54,7 @@ arch_set_bit(long nr,

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 7, 2020 12:44:44 AM PDT, David Laight wrote: >From: Brian Gerst >> Sent: 07 May 2020 07:18 >... >> > --- a/arch/x86/include/asm/bitops.h >> > +++ b/arch/x86/include/asm/bitops.h >> > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long >*addr) >> > if

RE: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread David Laight
From: Brian Gerst > Sent: 07 May 2020 07:18 ... > > --- a/arch/x86/include/asm/bitops.h > > +++ b/arch/x86/include/asm/bitops.h > > @@ -54,7 +54,7 @@ arch_set_bit(long nr, volatile unsigned long *addr) > > if (__builtin_constant_p(nr)) { > > asm volatile(LOCK_PREFIX "orb

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread hpa
On May 6, 2020 11:18:09 PM PDT, Brian Gerst wrote: >On Tue, May 5, 2020 at 1:47 PM Nick Desaulniers > wrote: >> >> From: Sedat Dilek >> >> It turns out that if your config tickles __builtin_constant_p via >> differences in choices to inline or not, this now produces invalid >> assembly: >> >> $

Re: [PATCH] x86: bitops: fix build regression

2020-05-07 Thread Brian Gerst
On Tue, May 5, 2020 at 1:47 PM Nick Desaulniers wrote: > > From: Sedat Dilek > > It turns out that if your config tickles __builtin_constant_p via > differences in choices to inline or not, this now produces invalid > assembly: > > $ cat foo.c > long a(long b, long c) { > asm("orb\t%1, %0" :

Re: [PATCH] x86: bitops: fix build regression

2020-05-06 Thread Ilie Halip
Hi Nick, > Ilie, may I put your authorship and signed off by tag on the V2? Yes, of course. With the current global situation I took some time off and didn't follow the latest discussion. Feel free to credit/rework the code as you see fit. Thanks, I.H.

Re: [PATCH] x86: bitops: fix build regression

2020-05-06 Thread Nick Desaulniers
On Tue, May 5, 2020 at 9:30 PM Nathan Chancellor wrote: > > On Tue, May 05, 2020 at 10:44:22AM -0700, Nick Desaulniers wrote: > > This is easily reproducible via Clang+CONFIG_STAGING=y+CONFIG_VT6656=m, > > or Clang+allyesconfig. > > For what it's worth, I don't see this with allyesconfig. Oops,

Re: [PATCH] x86: bitops: fix build regression

2020-05-06 Thread Nathan Chancellor
On Tue, May 05, 2020 at 09:30:28PM -0700, Nathan Chancellor wrote: > On Tue, May 05, 2020 at 10:44:22AM -0700, Nick Desaulniers wrote: > > From: Sedat Dilek > > > > It turns out that if your config tickles __builtin_constant_p via > > differences in choices to inline or not, this now produces

Re: [PATCH] x86: bitops: fix build regression

2020-05-06 Thread Sedat Dilek
On Wed, May 6, 2020 at 6:30 AM Nathan Chancellor wrote: > > On Tue, May 05, 2020 at 10:44:22AM -0700, Nick Desaulniers wrote: > > From: Sedat Dilek > > > > It turns out that if your config tickles __builtin_constant_p via > > differences in choices to inline or not, this now produces invalid > >

Re: [PATCH] x86: bitops: fix build regression

2020-05-05 Thread Nathan Chancellor
On Tue, May 05, 2020 at 10:44:22AM -0700, Nick Desaulniers wrote: > From: Sedat Dilek > > It turns out that if your config tickles __builtin_constant_p via > differences in choices to inline or not, this now produces invalid > assembly: > > $ cat foo.c > long a(long b, long c) { >

Re: [PATCH] x86: bitops: fix build regression

2020-05-05 Thread Nick Desaulniers
On Tue, May 5, 2020 at 11:07 AM wrote: > > On May 5, 2020 10:44:22 AM PDT, Nick Desaulniers > wrote: > >From: Sedat Dilek > > > >It turns out that if your config tickles __builtin_constant_p via > >differences in choices to inline or not, this now produces invalid > >assembly: > > > >$ cat

Re: [PATCH] x86: bitops: fix build regression

2020-05-05 Thread hpa
On May 5, 2020 10:44:22 AM PDT, Nick Desaulniers wrote: >From: Sedat Dilek > >It turns out that if your config tickles __builtin_constant_p via >differences in choices to inline or not, this now produces invalid >assembly: > >$ cat foo.c >long a(long b, long c) { > asm("orb\t%1, %0" : "+q"(c):