Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332779 > unrolled thread

[PATCH] [media] zl10353: use div_u64 instead of do_div

Back to article view | Back to linux.kernel

Contents

  [PATCH] [media] zl10353: use div_u64 instead of do_div Arnd Bergmann <arnd@arndb.de> - 2016-02-12 15:30 +0100
    Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-02-12 17:40 +0100
      Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Arnd Bergmann <arnd@arndb.de> - 2016-02-12 18:10 +0100
        Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-12 19:30 +0100
          Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Arnd Bergmann <arnd@arndb.de> - 2016-02-12 22:10 +0100
            Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-12 22:40 +0100
              Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Arnd Bergmann <arnd@arndb.de> - 2016-02-12 22:50 +0100
            Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-13 09:50 +0100
              Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-13 23:00 +0100
                Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-14 09:00 +0100
                  Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-02-14 18:00 +0100
                    Re: [PATCH] [media] zl10353: use div_u64 instead of do_div Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-02-14 20:10 +0100

#1332779 — [PATCH] [media] zl10353: use div_u64 instead of do_div

I noticed a build error in some randconfig builds in the zl10353 driver:

dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'

The problem can be tracked down to the use of -fprofile-arcs (using
CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
on gcc version 4.9 or higher, when it fails to reliably optimize
constant expressions.

Using div_u64() instead of do_div() makes the code slightly more
readable by both humans and by gcc, which gives the compiler enough
of a break to figure it all out.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/dvb-frontends/zl10353.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/zl10353.c b/drivers/media/dvb-frontends/zl10353.c
index ef9764a02d4c..160c88710553 100644
--- a/drivers/media/dvb-frontends/zl10353.c
+++ b/drivers/media/dvb-frontends/zl10353.c
@@ -135,8 +135,7 @@ static void zl10353_calc_nominal_rate(struct dvb_frontend *fe,
 
 	value = (u64)10 * (1 << 23) / 7 * 125;
 	value = (bw * value) + adc_clock / 2;
-	do_div(value, adc_clock);
-	*nominal_rate = value;
+	*nominal_rate = div_u64(value, adc_clock);
 
 	dprintk("%s: bw %d, adc_clock %d => 0x%x\n",
 		__func__, bw, adc_clock, *nominal_rate);
@@ -163,8 +162,7 @@ static void zl10353_calc_input_freq(struct dvb_frontend *fe,
 		if (ife > adc_clock / 2)
 			ife = adc_clock - ife;
 	}
-	value = (u64)65536 * ife + adc_clock / 2;
-	do_div(value, adc_clock);
+	value = div_u64((u64)65536 * ife + adc_clock / 2, adc_clock);
 	*input_freq = -value;
 
 	dprintk("%s: if2 %d, ife %d, adc_clock %d => %d / 0x%x\n",
-- 
2.7.0

[toc] | [next] | [standalone]

#1332885

Em Fri, 12 Feb 2016 15:27:18 +0100
Arnd Bergmann <arnd@arndb.de> escreveu:

> I noticed a build error in some randconfig builds in the zl10353 driver:
> 
> dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
> dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
> 
> The problem can be tracked down to the use of -fprofile-arcs (using
> CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
> on gcc version 4.9 or higher, when it fails to reliably optimize
> constant expressions.
> 
> Using div_u64() instead of do_div() makes the code slightly more
> readable by both humans and by gcc, which gives the compiler enough
> of a break to figure it all out.

I'm not against this patch, but we have 94 occurrences of do_div() 
just at the media subsystem. If this is failing here, it would likely
fail with other drivers. So, I guess we should either fix do_div() or
convert all such occurrences to do_div64().

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/dvb-frontends/zl10353.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/zl10353.c b/drivers/media/dvb-frontends/zl10353.c
> index ef9764a02d4c..160c88710553 100644
> --- a/drivers/media/dvb-frontends/zl10353.c
> +++ b/drivers/media/dvb-frontends/zl10353.c
> @@ -135,8 +135,7 @@ static void zl10353_calc_nominal_rate(struct dvb_frontend *fe,
>  
>  	value = (u64)10 * (1 << 23) / 7 * 125;
>  	value = (bw * value) + adc_clock / 2;
> -	do_div(value, adc_clock);
> -	*nominal_rate = value;
> +	*nominal_rate = div_u64(value, adc_clock);
>  
>  	dprintk("%s: bw %d, adc_clock %d => 0x%x\n",
>  		__func__, bw, adc_clock, *nominal_rate);
> @@ -163,8 +162,7 @@ static void zl10353_calc_input_freq(struct dvb_frontend *fe,
>  		if (ife > adc_clock / 2)
>  			ife = adc_clock - ife;
>  	}
> -	value = (u64)65536 * ife + adc_clock / 2;
> -	do_div(value, adc_clock);
> +	value = div_u64((u64)65536 * ife + adc_clock / 2, adc_clock);
>  	*input_freq = -value;
>  
>  	dprintk("%s: if2 %d, ife %d, adc_clock %d => %d / 0x%x\n",

[toc] | [prev] | [next] | [standalone]

#1332914

On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
> Em Fri, 12 Feb 2016 15:27:18 +0100
> Arnd Bergmann <arnd@arndb.de> escreveu:
> 
> > I noticed a build error in some randconfig builds in the zl10353 driver:
> > 
> > dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
> > dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
> > 
> > The problem can be tracked down to the use of -fprofile-arcs (using
> > CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
> > on gcc version 4.9 or higher, when it fails to reliably optimize
> > constant expressions.
> > 
> > Using div_u64() instead of do_div() makes the code slightly more
> > readable by both humans and by gcc, which gives the compiler enough
> > of a break to figure it all out.
> 
> I'm not against this patch, but we have 94 occurrences of do_div() 
> just at the media subsystem. If this is failing here, it would likely
> fail with other drivers. So, I guess we should either fix do_div() or
> convert all such occurrences to do_div64().

I agree that it's possible that the same problem exists elsewhere, but this is
the only one that I ever saw (in five ranconfig builds out of 8035 last week).

I also tried changing do_div() to be an inline function with just a small
macro wrapper around it for the odd calling conventions, which also made this
error go away. I would assume that Nico had a good reason for doing do_div()
the way he did. In some other files, I saw the object code grow by a few
instructions, but the examples I looked at were otherwise identical.

I can imagine that there might be cases where the constant-argument optimization
of do_div fails when we go through an inline function in some combination
of Kconfig options and compiler version, though I don't think that was
the case here.

Nico, any other thoughts on this?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1333007

On Fri, 12 Feb 2016, Arnd Bergmann wrote:

> On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
> > Em Fri, 12 Feb 2016 15:27:18 +0100
> > Arnd Bergmann <arnd@arndb.de> escreveu:
> > 
> > > I noticed a build error in some randconfig builds in the zl10353 driver:
> > > 
> > > dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
> > > dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
> > > 
> > > The problem can be tracked down to the use of -fprofile-arcs (using
> > > CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
> > > on gcc version 4.9 or higher, when it fails to reliably optimize
> > > constant expressions.
> > > 
> > > Using div_u64() instead of do_div() makes the code slightly more
> > > readable by both humans and by gcc, which gives the compiler enough
> > > of a break to figure it all out.
> > 
> > I'm not against this patch, but we have 94 occurrences of do_div() 
> > just at the media subsystem. If this is failing here, it would likely
> > fail with other drivers. So, I guess we should either fix do_div() or
> > convert all such occurrences to do_div64().
> 
> I agree that it's possible that the same problem exists elsewhere, but this is
> the only one that I ever saw (in five ranconfig builds out of 8035 last week).
> 
> I also tried changing do_div() to be an inline function with just a small
> macro wrapper around it for the odd calling conventions, which also made this
> error go away. I would assume that Nico had a good reason for doing do_div()
> the way he did.

The do_div() calling convention predates my work on it.  I assume it was 
originally done this way to better map onto the x86 instruction.

> In some other files, I saw the object code grow by a few
> instructions, but the examples I looked at were otherwise identical.
> 
> I can imagine that there might be cases where the constant-argument optimization
> of do_div fails when we go through an inline function in some combination
> of Kconfig options and compiler version, though I don't think that was
> the case here.

What could be tried is to turn __div64_const32() into a static inline 
and see if that makes a difference with those gcc versions we currently 
accept.

> Nico, any other thoughts on this?

This is all related to the gcc bug for which I produced a test case 
here:

http://article.gmane.org/gmane.linux.kernel.cross-arch/29801

Do you know if this is fixed in recent gcc?


Nicolas

[toc] | [prev] | [next] | [standalone]

#1333123

On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
> On Fri, 12 Feb 2016, Arnd Bergmann wrote:
> 
> > On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
> > > Em Fri, 12 Feb 2016 15:27:18 +0100
> > > Arnd Bergmann <arnd@arndb.de> escreveu:
> > > 
> > > > I noticed a build error in some randconfig builds in the zl10353 driver:
> > > > 
> > > > dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
> > > > dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
> > > > 
> > > > The problem can be tracked down to the use of -fprofile-arcs (using
> > > > CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
> > > > on gcc version 4.9 or higher, when it fails to reliably optimize
> > > > constant expressions.
> > > > 
> > > > Using div_u64() instead of do_div() makes the code slightly more
> > > > readable by both humans and by gcc, which gives the compiler enough
> > > > of a break to figure it all out.
> > > 
> > > I'm not against this patch, but we have 94 occurrences of do_div() 
> > > just at the media subsystem. If this is failing here, it would likely
> > > fail with other drivers. So, I guess we should either fix do_div() or
> > > convert all such occurrences to do_div64().
> > 
> > I agree that it's possible that the same problem exists elsewhere, but this is
> > the only one that I ever saw (in five ranconfig builds out of 8035 last week).
> > 
> > I also tried changing do_div() to be an inline function with just a small
> > macro wrapper around it for the odd calling conventions, which also made this
> > error go away. I would assume that Nico had a good reason for doing do_div()
> > the way he did.
> 
> The do_div() calling convention predates my work on it.  I assume it was 
> originally done this way to better map onto the x86 instruction.

Right, this goes back to the dawn of time.

> > In some other files, I saw the object code grow by a few
> > instructions, but the examples I looked at were otherwise identical.
> > 
> > I can imagine that there might be cases where the constant-argument optimization
> > of do_div fails when we go through an inline function in some combination
> > of Kconfig options and compiler version, though I don't think that was
> > the case here.
> 
> What could be tried is to turn __div64_const32() into a static inline 
> and see if that makes a difference with those gcc versions we currently 
> accept.
> 
> > Nico, any other thoughts on this?
> 
> This is all related to the gcc bug for which I produced a test case 
> here:
> 
> http://article.gmane.org/gmane.linux.kernel.cross-arch/29801
> 
> Do you know if this is fixed in recent gcc?

I have a fairly recent gcc, but I also never got around to submit
it properly.

However, I did stumble over an older patch I did now, which I could
not remember what it was good for. It does fix the problem, and
it seems to be a better solution.

	Arnd

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index b5acbb404854..b5ff9881bef8 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
  */
 #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
 #define __trace_if(cond) \
-	if (__builtin_constant_p((cond)) ? !!(cond) :			\
+	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
 	({								\
 		int ______r;						\
 		static struct ftrace_branch_data			\

[toc] | [prev] | [next] | [standalone]

#1333144

On Fri, 12 Feb 2016, Arnd Bergmann wrote:

> On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
> > This is all related to the gcc bug for which I produced a test case 
> > here:
> > 
> > http://article.gmane.org/gmane.linux.kernel.cross-arch/29801
> > 
> > Do you know if this is fixed in recent gcc?
> 
> I have a fairly recent gcc, but I also never got around to submit
> it properly.
> 
> However, I did stumble over an older patch I did now, which I could
> not remember what it was good for. It does fix the problem, and
> it seems to be a better solution.

WTF?

Hmmm... it apparently doesn't fix it if I apply this change to the gcc 
test case.


> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index b5acbb404854..b5ff9881bef8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   */
>  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
>  #define __trace_if(cond) \
> -	if (__builtin_constant_p((cond)) ? !!(cond) :			\
> +	if (__builtin_constant_p(!!(cond)) ? !!(cond) :			\
>  	({								\
>  		int ______r;						\
>  		static struct ftrace_branch_data			\
> 
> 

[toc] | [prev] | [next] | [standalone]

#1333152

On Friday 12 February 2016 16:38:53 Nicolas Pitre wrote:
> On Fri, 12 Feb 2016, Arnd Bergmann wrote:
> 
> > On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
> > > This is all related to the gcc bug for which I produced a test case 
> > > here:
> > > 
> > > http://article.gmane.org/gmane.linux.kernel.cross-arch/29801
> > > 
> > > Do you know if this is fixed in recent gcc?
> > 
> > I have a fairly recent gcc, but I also never got around to submit
> > it properly.
> > 
> > However, I did stumble over an older patch I did now, which I could
> > not remember what it was good for. It does fix the problem, and
> > it seems to be a better solution.
> 
> WTF?

Even better, it also fixes this one:

drivers/mtd/chips/cfi_cmdset_0020.c: In function 'cfi_staa_write_buffers':
drivers/mtd/chips/cfi_cmdset_0020.c:651:1: error: the frame size of 1064 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

I have not even looked what that is, I only saw show up the other day.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1333290

On 12 February 2016 at 22:01, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 12 February 2016 13:21:33 Nicolas Pitre wrote:
>> On Fri, 12 Feb 2016, Arnd Bergmann wrote:
>>
>> > On Friday 12 February 2016 14:32:20 Mauro Carvalho Chehab wrote:
>> > > Em Fri, 12 Feb 2016 15:27:18 +0100
>> > > Arnd Bergmann <arnd@arndb.de> escreveu:
>> > >
>> > > > I noticed a build error in some randconfig builds in the zl10353 driver:
>> > > >
>> > > > dvb-frontends/zl10353.c:138: undefined reference to `____ilog2_NaN'
>> > > > dvb-frontends/zl10353.c:138: undefined reference to `__aeabi_uldivmod'
>> > > >
>> > > > The problem can be tracked down to the use of -fprofile-arcs (using
>> > > > CONFIG_GCOV_PROFILE_ALL) in combination with CONFIG_PROFILE_ALL_BRANCHES
>> > > > on gcc version 4.9 or higher, when it fails to reliably optimize
>> > > > constant expressions.
>> > > >
>> > > > Using div_u64() instead of do_div() makes the code slightly more
>> > > > readable by both humans and by gcc, which gives the compiler enough
>> > > > of a break to figure it all out.
>> > >
>> > > I'm not against this patch, but we have 94 occurrences of do_div()
>> > > just at the media subsystem. If this is failing here, it would likely
>> > > fail with other drivers. So, I guess we should either fix do_div() or
>> > > convert all such occurrences to do_div64().
>> >
>> > I agree that it's possible that the same problem exists elsewhere, but this is
>> > the only one that I ever saw (in five ranconfig builds out of 8035 last week).
>> >
>> > I also tried changing do_div() to be an inline function with just a small
>> > macro wrapper around it for the odd calling conventions, which also made this
>> > error go away. I would assume that Nico had a good reason for doing do_div()
>> > the way he did.
>>
>> The do_div() calling convention predates my work on it.  I assume it was
>> originally done this way to better map onto the x86 instruction.
>
> Right, this goes back to the dawn of time.
>
>> > In some other files, I saw the object code grow by a few
>> > instructions, but the examples I looked at were otherwise identical.
>> >
>> > I can imagine that there might be cases where the constant-argument optimization
>> > of do_div fails when we go through an inline function in some combination
>> > of Kconfig options and compiler version, though I don't think that was
>> > the case here.
>>
>> What could be tried is to turn __div64_const32() into a static inline
>> and see if that makes a difference with those gcc versions we currently
>> accept.
>>
>> > Nico, any other thoughts on this?
>>
>> This is all related to the gcc bug for which I produced a test case
>> here:
>>
>> http://article.gmane.org/gmane.linux.kernel.cross-arch/29801
>>
>> Do you know if this is fixed in recent gcc?
>
> I have a fairly recent gcc, but I also never got around to submit
> it properly.
>
> However, I did stumble over an older patch I did now, which I could
> not remember what it was good for. It does fix the problem, and
> it seems to be a better solution.
>
>         Arnd
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index b5acbb404854..b5ff9881bef8 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>   */
>  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
>  #define __trace_if(cond) \
> -       if (__builtin_constant_p((cond)) ? !!(cond) :                   \
> +       if (__builtin_constant_p(!!(cond)) ? !!(cond) :                 \
>         ({                                                              \
>                 int ______r;                                            \
>                 static struct ftrace_branch_data                        \
>

I remember seeing this patch, but I don't remember the exact context.
But when you think about it, !!cond can be a build time constant even
if cond is not, as long as you can prove statically that cond != 0. So
I think this change is obviously correct, and an improvement since it
will remove the profiling overhead of branches that are not true
branches in the first place.

[toc] | [prev] | [next] | [standalone]

#1333369

On Sat, 13 Feb 2016, Ard Biesheuvel wrote:

> On 12 February 2016 at 22:01, Arnd Bergmann <arnd@arndb.de> wrote:
> > However, I did stumble over an older patch I did now, which I could
> > not remember what it was good for. It does fix the problem, and
> > it seems to be a better solution.
> >
> >         Arnd
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index b5acbb404854..b5ff9881bef8 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> >   */
> >  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
> >  #define __trace_if(cond) \
> > -       if (__builtin_constant_p((cond)) ? !!(cond) :                   \
> > +       if (__builtin_constant_p(!!(cond)) ? !!(cond) :                 \
> >         ({                                                              \
> >                 int ______r;                                            \
> >                 static struct ftrace_branch_data                        \
> >
> 
> I remember seeing this patch, but I don't remember the exact context.
> But when you think about it, !!cond can be a build time constant even
> if cond is not, as long as you can prove statically that cond != 0. So

You're right.  I just tested it and to my surprise gcc is smart enough 
to figure that case out.

> I think this change is obviously correct, and an improvement since it
> will remove the profiling overhead of branches that are not true
> branches in the first place.

Indeed.


Nicolas

[toc] | [prev] | [next] | [standalone]

#1333453

On 13 February 2016 at 22:57, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
>
>> On 12 February 2016 at 22:01, Arnd Bergmann <arnd@arndb.de> wrote:
>> > However, I did stumble over an older patch I did now, which I could
>> > not remember what it was good for. It does fix the problem, and
>> > it seems to be a better solution.
>> >
>> >         Arnd
>> >
>> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> > index b5acbb404854..b5ff9881bef8 100644
>> > --- a/include/linux/compiler.h
>> > +++ b/include/linux/compiler.h
>> > @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>> >   */
>> >  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
>> >  #define __trace_if(cond) \
>> > -       if (__builtin_constant_p((cond)) ? !!(cond) :                   \
>> > +       if (__builtin_constant_p(!!(cond)) ? !!(cond) :                 \
>> >         ({                                                              \
>> >                 int ______r;                                            \
>> >                 static struct ftrace_branch_data                        \
>> >
>>
>> I remember seeing this patch, but I don't remember the exact context.
>> But when you think about it, !!cond can be a build time constant even
>> if cond is not, as long as you can prove statically that cond != 0. So
>
> You're right.  I just tested it and to my surprise gcc is smart enough
> to figure that case out.
>
>> I think this change is obviously correct, and an improvement since it
>> will remove the profiling overhead of branches that are not true
>> branches in the first place.
>
> Indeed.
>

... and perhaps we should not evaluate cond twice either?

[toc] | [prev] | [next] | [standalone]

#1333519

On Sun, 14 Feb 2016, Ard Biesheuvel wrote:

> On 13 February 2016 at 22:57, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> > On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
> >
> >> On 12 February 2016 at 22:01, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > However, I did stumble over an older patch I did now, which I could
> >> > not remember what it was good for. It does fix the problem, and
> >> > it seems to be a better solution.
> >> >
> >> >         Arnd
> >> >
> >> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> >> > index b5acbb404854..b5ff9881bef8 100644
> >> > --- a/include/linux/compiler.h
> >> > +++ b/include/linux/compiler.h
> >> > @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> >> >   */
> >> >  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
> >> >  #define __trace_if(cond) \
> >> > -       if (__builtin_constant_p((cond)) ? !!(cond) :                   \
> >> > +       if (__builtin_constant_p(!!(cond)) ? !!(cond) :                 \
> >> >         ({                                                              \
> >> >                 int ______r;                                            \
> >> >                 static struct ftrace_branch_data                        \
> >> >
> >>
> >> I remember seeing this patch, but I don't remember the exact context.
> >> But when you think about it, !!cond can be a build time constant even
> >> if cond is not, as long as you can prove statically that cond != 0. So
> >
> > You're right.  I just tested it and to my surprise gcc is smart enough
> > to figure that case out.
> >
> >> I think this change is obviously correct, and an improvement since it
> >> will remove the profiling overhead of branches that are not true
> >> branches in the first place.
> >
> > Indeed.
> >
> 
> ... and perhaps we should not evaluate cond twice either?

It is not. The value of the argument to __builtin_constant_p() is not 
itself evaluated and therefore does not produce side effects.


Nicolas

[toc] | [prev] | [next] | [standalone]

#1333539

On 14 February 2016 at 17:52, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> On Sun, 14 Feb 2016, Ard Biesheuvel wrote:
>
>> On 13 February 2016 at 22:57, Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>> > On Sat, 13 Feb 2016, Ard Biesheuvel wrote:
>> >
>> >> On 12 February 2016 at 22:01, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> > However, I did stumble over an older patch I did now, which I could
>> >> > not remember what it was good for. It does fix the problem, and
>> >> > it seems to be a better solution.
>> >> >
>> >> >         Arnd
>> >> >
>> >> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
>> >> > index b5acbb404854..b5ff9881bef8 100644
>> >> > --- a/include/linux/compiler.h
>> >> > +++ b/include/linux/compiler.h
>> >> > @@ -148,7 +148,7 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
>> >> >   */
>> >> >  #define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
>> >> >  #define __trace_if(cond) \
>> >> > -       if (__builtin_constant_p((cond)) ? !!(cond) :                   \
>> >> > +       if (__builtin_constant_p(!!(cond)) ? !!(cond) :                 \
>> >> >         ({                                                              \
>> >> >                 int ______r;                                            \
>> >> >                 static struct ftrace_branch_data                        \
>> >> >
>> >>
>> >> I remember seeing this patch, but I don't remember the exact context.
>> >> But when you think about it, !!cond can be a build time constant even
>> >> if cond is not, as long as you can prove statically that cond != 0. So
>> >
>> > You're right.  I just tested it and to my surprise gcc is smart enough
>> > to figure that case out.
>> >
>> >> I think this change is obviously correct, and an improvement since it
>> >> will remove the profiling overhead of branches that are not true
>> >> branches in the first place.
>> >
>> > Indeed.
>> >
>>
>> ... and perhaps we should not evaluate cond twice either?
>
> It is not. The value of the argument to __builtin_constant_p() is not
> itself evaluated and therefore does not produce side effects.
>

Interesting, thanks for clarifying.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web