* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 14:27 ` Arnd Bergmann
0 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 14:27 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-arm-kernel, Arnd Bergmann, Nicolas Pitre, Stefan Richter,
linux-media, linux-kernel
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 14:27 ` Arnd Bergmann
0 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 14:27 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 14:27 ` Arnd Bergmann
@ 2016-02-12 16:32 ` Mauro Carvalho Chehab
-1 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2016-02-12 16:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, Nicolas Pitre, Stefan Richter, linux-media,
linux-kernel
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",
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 16:32 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 29+ messages in thread
From: Mauro Carvalho Chehab @ 2016-02-12 16:32 UTC (permalink / raw)
To: linux-arm-kernel
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",
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 16:32 ` Mauro Carvalho Chehab
@ 2016-02-12 17:04 ` Arnd Bergmann
-1 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 17:04 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-arm-kernel, Nicolas Pitre, Stefan Richter, linux-media,
linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 17:04 ` Arnd Bergmann
0 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 17:04 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 17:04 ` Arnd Bergmann
@ 2016-02-12 18:21 ` Nicolas Pitre
-1 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-12 18:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, linux-arm-kernel, Stefan Richter,
linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 18:21 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-12 18:21 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 18:21 ` Nicolas Pitre
@ 2016-02-12 21:01 ` Arnd Bergmann
-1 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:01 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Mauro Carvalho Chehab, linux-arm-kernel, Stefan Richter,
linux-media, linux-kernel
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 \
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 21:01 ` Arnd Bergmann
0 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:01 UTC (permalink / raw)
To: linux-arm-kernel
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 \
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 21:01 ` Arnd Bergmann
@ 2016-02-12 21:38 ` Nicolas Pitre
-1 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-12 21:38 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Mauro Carvalho Chehab, linux-arm-kernel, Stefan Richter,
linux-media, linux-kernel
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 \
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 21:38 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-12 21:38 UTC (permalink / raw)
To: linux-arm-kernel
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 \
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 21:38 ` Nicolas Pitre
@ 2016-02-12 21:45 ` Arnd Bergmann
-1 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:45 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Nicolas Pitre, linux-kernel, Stefan Richter, linux-media,
Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-12 21:45 ` Arnd Bergmann
0 siblings, 0 replies; 29+ messages in thread
From: Arnd Bergmann @ 2016-02-12 21:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-12 21:01 ` Arnd Bergmann
(?)
@ 2016-02-13 8:39 ` Ard Biesheuvel
-1 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-13 8:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Nicolas Pitre, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-13 8:39 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-13 8:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Nicolas Pitre, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-13 8:39 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-13 8:39 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-13 8:39 ` Ard Biesheuvel
(?)
@ 2016-02-13 21:57 ` Nicolas Pitre
-1 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-13 21:57 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-13 21:57 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-13 21:57 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-13 21:57 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-13 21:57 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-13 21:57 ` Nicolas Pitre
(?)
@ 2016-02-14 7:57 ` Ard Biesheuvel
-1 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 7:57 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 7:57 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 7:57 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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?
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 7:57 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 7:57 UTC (permalink / raw)
To: linux-arm-kernel
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?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-14 7:57 ` Ard Biesheuvel
(?)
@ 2016-02-14 16:52 ` Nicolas Pitre
-1 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-14 16:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 16:52 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-14 16:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 16:52 ` Nicolas Pitre
0 siblings, 0 replies; 29+ messages in thread
From: Nicolas Pitre @ 2016-02-14 16:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
2016-02-14 16:52 ` Nicolas Pitre
(?)
@ 2016-02-14 19:06 ` Ard Biesheuvel
-1 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 19:06 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 19:06 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 19:06 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Arnd Bergmann, linux-kernel, Stefan Richter, linux-media,
linux-arm-kernel, Mauro Carvalho Chehab
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] [media] zl10353: use div_u64 instead of do_div
@ 2016-02-14 19:06 ` Ard Biesheuvel
0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-02-14 19:06 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-02-14 19:06 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 14:27 [PATCH] [media] zl10353: use div_u64 instead of do_div Arnd Bergmann
2016-02-12 14:27 ` Arnd Bergmann
2016-02-12 16:32 ` Mauro Carvalho Chehab
2016-02-12 16:32 ` Mauro Carvalho Chehab
2016-02-12 17:04 ` Arnd Bergmann
2016-02-12 17:04 ` Arnd Bergmann
2016-02-12 18:21 ` Nicolas Pitre
2016-02-12 18:21 ` Nicolas Pitre
2016-02-12 21:01 ` Arnd Bergmann
2016-02-12 21:01 ` Arnd Bergmann
2016-02-12 21:38 ` Nicolas Pitre
2016-02-12 21:38 ` Nicolas Pitre
2016-02-12 21:45 ` Arnd Bergmann
2016-02-12 21:45 ` Arnd Bergmann
2016-02-13 8:39 ` Ard Biesheuvel
2016-02-13 8:39 ` Ard Biesheuvel
2016-02-13 8:39 ` Ard Biesheuvel
2016-02-13 21:57 ` Nicolas Pitre
2016-02-13 21:57 ` Nicolas Pitre
2016-02-13 21:57 ` Nicolas Pitre
2016-02-14 7:57 ` Ard Biesheuvel
2016-02-14 7:57 ` Ard Biesheuvel
2016-02-14 7:57 ` Ard Biesheuvel
2016-02-14 16:52 ` Nicolas Pitre
2016-02-14 16:52 ` Nicolas Pitre
2016-02-14 16:52 ` Nicolas Pitre
2016-02-14 19:06 ` Ard Biesheuvel
2016-02-14 19:06 ` Ard Biesheuvel
2016-02-14 19:06 ` Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.