* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
[not found] ` <Y1ercgaqQwfqt42U@ashyti-mobl2.lan>
@ 2022-10-25 16:46 ` Nick Desaulniers
2022-10-25 18:45 ` Dixit, Ashutosh
1 sibling, 0 replies; 12+ messages in thread
From: Nick Desaulniers @ 2022-10-25 16:46 UTC (permalink / raw)
To: Andi Shyti
Cc: Dixit, Ashutosh, Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm, LKML
Start of lore thread for context:
https://lore.kernel.org/intel-gfx/20221024210953.1572998-1-gwan-gyeong.mun@intel.com/
On Tue, Oct 25, 2022 at 2:25 AM Andi Shyti <andi.shyti@linux.intel.com> wrote:
>
> Hi Ashutosh,
>
> > > drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (field_msk)))' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >
> > What is 18446744073709551615? You may want to limit the length of this line
> > or checkpatch doesn't complain?
>
> yeah! I am not a clang user, and this must be some ugly error
> output. I don't think it makes sense to break it, though.
>
> > > bits_to_set = FIELD_PREP(field_msk, nval);
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> > > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > > ./include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > > ./include/linux/compiler_types.h:357:22: note: expanded from macro 'compiletime_assert'
> > > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/compiler_types.h:345:23: note: expanded from macro '_compiletime_assert'
> > > __compiletime_assert(condition, msg, prefix, suffix)
> > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/compiler_types.h:337:9: note: expanded from macro '__compiletime_assert'
> > > if (!(condition)) \
> > >
> > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_hwmon.c | 12 +++---------
> > > 1 file changed, 3 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > index 9e9781493025..782a621b1928 100644
> > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > >
> > > static void
> > > hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > > - u32 field_msk, int nshift,
> > > - unsigned int scale_factor, long lval)
> > > + int nshift, unsigned int scale_factor, long lval)
> > > {
> > > u32 nval;
> > > - u32 bits_to_clear;
> > > - u32 bits_to_set;
> > >
> > > /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> > >
> > > - bits_to_clear = field_msk;
> > > - bits_to_set = FIELD_PREP(field_msk, nval);
> > > -
> > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> > > - bits_to_clear, bits_to_set);
> > > + PKG_PWR_LIM_1,
> > > + FIELD_PREP(PKG_PWR_LIM_1, nval));
> >
> > I don't want to give up so easily. We might have future uses for the
> > function where we want field_msk to be passed into the function (rather
> > than set inside the function as in this patch).
> >
> > Do we understand what clang is complaining about? And why this compiles
> > with gcc?
>
> Because we are not compiling the builtin functions with gcc but
> gcc has support for them. The FIELD_PREP checks if the first
> parameter is a constant:
>
> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),
>
> where _mask was our field_mask, but we ignore it. Apparently
> clang doesn't.
So we've been in this code before. I'm having vague memories of
commit 444da3f52407 ("bitfield.h: don't compile-time validate _val in
FIELD_FIT")
But looking at the first __builtin_constant_p check in
__BF_FIELD_CHECK, I'm curious if that might need to be __is_constexpr
rather than __builtin_constant_p; a change like
commit 4d45bc82df66 ("coresight: etm4x: avoid build failure with
unrolled loops")
__builtin_constant_p is evaluated after most optimizations have run;
__is_constexpr must be evaluated by compiler front ends during
semantic analysis.
But reading through the full lore thread, it sounds like Jani has
another suggestion to try instead.
https://lore.kernel.org/intel-gfx/87eduwdllr.fsf@intel.com/
Please re-cc us and our list if that doesn't work out.
>
> If we want to stick to gcc only, then I still think the patch is
> correct for two reasons:
>
> 1. it's cleaner
> 2. we would get on with the job and if one day we will decide
> to suppport builtin functions in gcc as well, we will sleep
> peacefully :)
>
> > Copying llvm@lists.linux.dev too.
>
> maybe llvm folks have a better opinion.
>
> Thanks,
> Andi
>
> > Thanks.
> > --
> > Ashutosh
> >
> >
> > > }
> > >
> > > /*
> > > @@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
> > > case hwmon_power_max:
> > > hwm_field_scale_and_write(ddat,
> > > hwmon->rg.pkg_rapl_limit,
> > > - PKG_PWR_LIM_1,
> > > hwmon->scl_shift_power,
> > > SF_POWER, val);
> > > return 0;
> > > --
> > > 2.37.1
> > >
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
[not found] ` <Y1ercgaqQwfqt42U@ashyti-mobl2.lan>
2022-10-25 16:46 ` [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Nick Desaulniers
@ 2022-10-25 18:45 ` Dixit, Ashutosh
2022-10-26 0:18 ` Andi Shyti
1 sibling, 1 reply; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-10-25 18:45 UTC (permalink / raw)
To: Andi Shyti
Cc: Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm,
Nick Desaulniers, linux-kernel
On Tue, 25 Oct 2022 02:25:06 -0700, Andi Shyti wrote:
>
> Hi Ashutosh,
Hi Andi :)
> > > If a non-constant variable is used as the first argument of the FIELD_PREP
> > > macro, a build error occurs when using the clang compiler.
A "non-constant variable" does not seem to be the cause of the compile
error with clang, see below.
>
> > > drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (field_msk)))' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> >
> > What is 18446744073709551615? You may want to limit the length of this line
> > or checkpatch doesn't complain?
>
> yeah! I am not a clang user, and this must be some ugly error
> output. I don't think it makes sense to break it, though.
18446744073709551615 == ~0ull (see use in __BF_FIELD_CHECK).
>
> > > bits_to_set = FIELD_PREP(field_msk, nval);
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> > > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
also occurs here):
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
So it goes through previous checks including the "mask is not constant"
check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
after most optimizations have run" so by that time both compilers (gcc and
clang) have figured out that even though _mask is coming in as function
argument it is really the constant below:
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
But it is not clear why clang chokes on this "type of reg too small for
mask" check (and gcc doesn't) since everything is u32.
It is for this reason I want someone from llvm to chime in.
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > > ./include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > > ./include/linux/compiler_types.h:357:22: note: expanded from macro 'compiletime_assert'
> > > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/compiler_types.h:345:23: note: expanded from macro '_compiletime_assert'
> > > __compiletime_assert(condition, msg, prefix, suffix)
> > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ./include/linux/compiler_types.h:337:9: note: expanded from macro '__compiletime_assert'
> > > if (!(condition)) \
> > >
> > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_hwmon.c | 12 +++---------
> > > 1 file changed, 3 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > index 9e9781493025..782a621b1928 100644
> > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > >
> > > static void
> > > hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > > - u32 field_msk, int nshift,
> > > - unsigned int scale_factor, long lval)
> > > + int nshift, unsigned int scale_factor, long lval)
> > > {
> > > u32 nval;
> > > - u32 bits_to_clear;
> > > - u32 bits_to_set;
> > >
> > > /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> > >
> > > - bits_to_clear = field_msk;
> > > - bits_to_set = FIELD_PREP(field_msk, nval);
> > > -
> > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> > > - bits_to_clear, bits_to_set);
> > > + PKG_PWR_LIM_1,
> > > + FIELD_PREP(PKG_PWR_LIM_1, nval));
> >
> > I don't want to give up so easily. We might have future uses for the
> > function where we want field_msk to be passed into the function (rather
> > than set inside the function as in this patch).
> >
> > Do we understand what clang is complaining about? And why this compiles
> > with gcc?
>
> Because we are not compiling the builtin functions with gcc but
> gcc has support for them. The FIELD_PREP checks if the first
> parameter is a constant:
>
> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),
>
> where _mask was our field_mask, but we ignore it. Apparently
> clang doesn't.
So we have debunked this above.
> If we want to stick to gcc only, then I still think the patch is
> correct for two reasons:
>
> 1. it's cleaner
> 2. we would get on with the job and if one day we will decide
> to suppport builtin functions in gcc as well, we will sleep
> peacefully :)
I disagree with the patch even if we need to fix this in i915 (rather than
say change the headers or something in clang).
Note that hwm_field_scale_and_write() pairs with hwm_field_read_and_scale()
(they are basically a set/get pair) so it is desirable they have identical
arguments. This patch breaks that symmetry.
If we have to fix this in i915, I prefer the following patch (so just skip
the checks in FIELD_PREP):
@@ -112,7 +112,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
bits_to_clear = field_msk;
- bits_to_set = FIELD_PREP(field_msk, nval);
+ bits_to_set = (nval << __bf_shf(field_msk)) & field_msk;
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
But I'd wait to hear from clang/llvm folks first.
> > Copying llvm@lists.linux.dev too.
>
> maybe llvm folks have a better opinion.
>
Thanks.
--
Ashutosh
> > > }
> > >
> > > /*
> > > @@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
> > > case hwmon_power_max:
> > > hwm_field_scale_and_write(ddat,
> > > hwmon->rg.pkg_rapl_limit,
> > > - PKG_PWR_LIM_1,
> > > hwmon->scl_shift_power,
> > > SF_POWER, val);
> > > return 0;
> > > --
> > > 2.37.1
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-25 18:45 ` Dixit, Ashutosh
@ 2022-10-26 0:18 ` Andi Shyti
2022-10-27 16:35 ` Nick Desaulniers
0 siblings, 1 reply; 12+ messages in thread
From: Andi Shyti @ 2022-10-26 0:18 UTC (permalink / raw)
To: Dixit, Ashutosh
Cc: Andi Shyti, Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm,
Nick Desaulniers, linux-kernel
Hi Ashutosh,
> On Tue, 25 Oct 2022 02:25:06 -0700, Andi Shyti wrote:
> >
> > Hi Ashutosh,
>
> Hi Andi :)
>
> > > > If a non-constant variable is used as the first argument of the FIELD_PREP
> > > > macro, a build error occurs when using the clang compiler.
>
> A "non-constant variable" does not seem to be the cause of the compile
> error with clang, see below.
>
> >
> > > > drivers/gpu/drm/i915/i915_hwmon.c:115:16: error: result of comparison of constant 18446744073709551615 with expression of type 'typeof (_Generic((field_msk), char: (unsigned char)0, unsigned char: (unsigned char)0, signed char: (unsigned char)0, unsigned short: (unsigned short)0, short: (unsigned short)0, unsigned int: (unsigned int)0, int: (unsigned int)0, unsigned long: (unsigned long)0, long: (unsigned long)0, unsigned long long: (unsigned long long)0, long long: (unsigned long long)0, default: (field_msk)))' (aka 'unsigned int') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> > >
> > > What is 18446744073709551615? You may want to limit the length of this line
> > > or checkpatch doesn't complain?
> >
> > yeah! I am not a clang user, and this must be some ugly error
> > output. I don't think it makes sense to break it, though.
>
> 18446744073709551615 == ~0ull (see use in __BF_FIELD_CHECK).
I just wonder, then, where this number comes from, looks to me
like an ill formatted constant coming from the compiler
(definitely bigger than a ull).
> >
> > > > bits_to_set = FIELD_PREP(field_msk, nval);
> > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/bitfield.h:114:3: note: expanded from macro 'FIELD_PREP'
> > > > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>
> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
> also occurs here):
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull), \
> _pfx "type of reg too small for mask"); \
>
> So it goes through previous checks including the "mask is not constant"
> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
> after most optimizations have run" so by that time both compilers (gcc and
> clang) have figured out that even though _mask is coming in as function
> argument it is really the constant below:
>
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
I also thought that the compiler should have figured it out, but
then why we got that error, and I don't see how
"bf_cast_unsigned(_reg, ~0ull)" could fail.
> But it is not clear why clang chokes on this "type of reg too small for
> mask" check (and gcc doesn't) since everything is u32.
>
> It is for this reason I want someone from llvm to chime in.
>
> > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> > > > ./include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > > > ./include/linux/compiler_types.h:357:22: note: expanded from macro 'compiletime_assert'
> > > > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/compiler_types.h:345:23: note: expanded from macro '_compiletime_assert'
> > > > __compiletime_assert(condition, msg, prefix, suffix)
> > > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/compiler_types.h:337:9: note: expanded from macro '__compiletime_assert'
> > > > if (!(condition)) \
> > > >
> > > > Fixes: 99f55efb7911 ("drm/i915/hwmon: Power PL1 limit and TDP setting")
> > > > Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_hwmon.c | 12 +++---------
> > > > 1 file changed, 3 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > index 9e9781493025..782a621b1928 100644
> > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > @@ -101,21 +101,16 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > > >
> > > > static void
> > > > hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > > > - u32 field_msk, int nshift,
> > > > - unsigned int scale_factor, long lval)
> > > > + int nshift, unsigned int scale_factor, long lval)
> > > > {
> > > > u32 nval;
> > > > - u32 bits_to_clear;
> > > > - u32 bits_to_set;
> > > >
> > > > /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > > > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> > > >
> > > > - bits_to_clear = field_msk;
> > > > - bits_to_set = FIELD_PREP(field_msk, nval);
> > > > -
> > > > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> > > > - bits_to_clear, bits_to_set);
> > > > + PKG_PWR_LIM_1,
> > > > + FIELD_PREP(PKG_PWR_LIM_1, nval));
> > >
> > > I don't want to give up so easily. We might have future uses for the
> > > function where we want field_msk to be passed into the function (rather
> > > than set inside the function as in this patch).
> > >
> > > Do we understand what clang is complaining about? And why this compiles
> > > with gcc?
> >
> > Because we are not compiling the builtin functions with gcc but
> > gcc has support for them. The FIELD_PREP checks if the first
> > parameter is a constant:
> >
> > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),
> >
> > where _mask was our field_mask, but we ignore it. Apparently
> > clang doesn't.
>
> So we have debunked this above.
>
> > If we want to stick to gcc only, then I still think the patch is
> > correct for two reasons:
> >
> > 1. it's cleaner
> > 2. we would get on with the job and if one day we will decide
> > to suppport builtin functions in gcc as well, we will sleep
> > peacefully :)
>
> I disagree with the patch even if we need to fix this in i915 (rather than
> say change the headers or something in clang).
>
> Note that hwm_field_scale_and_write() pairs with hwm_field_read_and_scale()
> (they are basically a set/get pair) so it is desirable they have identical
> arguments. This patch breaks that symmetry.
OK, didn't see it! Makes sense.
> If we have to fix this in i915, I prefer the following patch (so just skip
> the checks in FIELD_PREP):
>
> @@ -112,7 +112,7 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> bits_to_clear = field_msk;
> - bits_to_set = FIELD_PREP(field_msk, nval);
> + bits_to_set = (nval << __bf_shf(field_msk)) & field_msk;
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
doesn't look pretty, though! :/
> But I'd wait to hear from clang/llvm folks first.
Yeah! Looking forward to getting some ideas :)
Thanks, Ashutosh!
Andi
> > > Copying llvm@lists.linux.dev too.
> >
> > maybe llvm folks have a better opinion.
> >
>
> Thanks.
> --
> Ashutosh
>
> > > > }
> > > >
> > > > /*
> > > > @@ -406,7 +401,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val)
> > > > case hwmon_power_max:
> > > > hwm_field_scale_and_write(ddat,
> > > > hwmon->rg.pkg_rapl_limit,
> > > > - PKG_PWR_LIM_1,
> > > > hwmon->scl_shift_power,
> > > > SF_POWER, val);
> > > > return 0;
> > > > --
> > > > 2.37.1
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-26 0:18 ` Andi Shyti
@ 2022-10-27 16:35 ` Nick Desaulniers
2022-10-27 16:53 ` Dixit, Ashutosh
0 siblings, 1 reply; 12+ messages in thread
From: Nick Desaulniers @ 2022-10-27 16:35 UTC (permalink / raw)
To: Gwan-gyeong Mun
Cc: Dixit, Ashutosh, anshuman.gupta, intel-gfx, llvm, linux-kernel,
Andi Shyti
On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti <andi.shyti@linux.intel.com> wrote:
>
> Hi Ashutosh,
>
> > But I'd wait to hear from clang/llvm folks first.
>
> Yeah! Looking forward to getting some ideas :)
Gwan-gyeong, which tree and set of configs are necessary to reproduce
the observed warning?
Warnings are treated as errors, so I don't want this breaking our CI.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-27 16:35 ` Nick Desaulniers
@ 2022-10-27 16:53 ` Dixit, Ashutosh
2022-10-27 17:16 ` Nick Desaulniers
0 siblings, 1 reply; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-10-27 16:53 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm, linux-kernel,
Andi Shyti
On Thu, 27 Oct 2022 09:35:24 -0700, Nick Desaulniers wrote:
>
Hi Nick,
> On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti <andi.shyti@linux.intel.com> wrote:
> >
> > Hi Ashutosh,
> >
> > > But I'd wait to hear from clang/llvm folks first.
> >
> > Yeah! Looking forward to getting some ideas :)
>
> Gwan-gyeong, which tree and set of configs are necessary to reproduce
> the observed warning?
>
> Warnings are treated as errors, so I don't want this breaking our CI.
The following or equivalent should do it:
git clone https://anongit.freedesktop.org/git/drm/drm-tip
git checkout drm-tip
Kernel config:
CONFIG_DRM_I915=m
CONFIG_HWMON=y
Files:
drivers/gpu/drm/i915/i915_hwmon.c/.h
Thanks for taking a look.
--
Ashutosh
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-27 16:53 ` Dixit, Ashutosh
@ 2022-10-27 17:16 ` Nick Desaulniers
2022-10-27 18:32 ` Dixit, Ashutosh
0 siblings, 1 reply; 12+ messages in thread
From: Nick Desaulniers @ 2022-10-27 17:16 UTC (permalink / raw)
To: Dixit, Ashutosh
Cc: Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm, linux-kernel,
Andi Shyti
On Thu, Oct 27, 2022 at 9:53 AM Dixit, Ashutosh
<ashutosh.dixit@intel.com> wrote:
>
> On Thu, 27 Oct 2022 09:35:24 -0700, Nick Desaulniers wrote:
> >
>
> Hi Nick,
>
> > On Tue, Oct 25, 2022 at 5:18 PM Andi Shyti <andi.shyti@linux.intel.com> wrote:
> > >
> > > Hi Ashutosh,
> > >
> > > > But I'd wait to hear from clang/llvm folks first.
> > >
> > > Yeah! Looking forward to getting some ideas :)
> >
> > Gwan-gyeong, which tree and set of configs are necessary to reproduce
> > the observed warning?
> >
> > Warnings are treated as errors, so I don't want this breaking our CI.
>
> The following or equivalent should do it:
>
> git clone https://anongit.freedesktop.org/git/drm/drm-tip
> git checkout drm-tip
>
> Kernel config:
> CONFIG_DRM_I915=m
> CONFIG_HWMON=y
>
> Files:
> drivers/gpu/drm/i915/i915_hwmon.c/.h
>
> Thanks for taking a look.
Thanks, I can repro now.
I haven't detangled the macro soup, but I noticed:
1. FIELD_PREP is defined in include/linux/bitfield.h which has the
following comment:
18 * Mask must be a compilation time constant.
2. hwm_field_scale_and_write only has one callsite.
The following patch works:
```
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 9e9781493025..6ac29d90b92a 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
static void
hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
*ddat, i915_reg_t rgadr,
/* Computation in 64-bits to avoid overflow. Round to nearest. */
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
- bits_to_clear = field_msk;
- bits_to_set = FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
attr, int chan, long val)
case hwmon_power_max:
hwm_field_scale_and_write(ddat,
hwmon->rg.pkg_rapl_limit,
- PKG_PWR_LIM_1,
hwmon->scl_shift_power,
SF_POWER, val);
return 0;
```
Though I'm not sure if you're planning to add further callsites of
hwm_field_scale_and_write with different field_masks?
Alternatively, (without the above diff),
```
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03d..6f40f12bcf89 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
#define _LINUX_BITFIELD_H
#include <linux/build_bug.h>
+#include <linux/const.h>
#include <asm/byteorder.h>
/*
@@ -62,7 +63,7 @@
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
- BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
+ BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
_pfx "mask is not constant"); \
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
```
will produce:
error: call to __compiletime_assert_407 declared with 'error'
attribute: FIELD_PREP: mask is not constant
I haven't tested if that change is also feasible (on top of fixing
this specific instance), but I think it might help avoid more of these
subtleties wrt. __builtin_constant_p that depende heavily on compiler,
compiler version, optimization level.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-27 17:16 ` Nick Desaulniers
@ 2022-10-27 18:32 ` Dixit, Ashutosh
2022-10-28 6:26 ` Gwan-gyeong Mun
2022-10-28 6:43 ` Gwan-gyeong Mun
0 siblings, 2 replies; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-10-27 18:32 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Gwan-gyeong Mun, anshuman.gupta, intel-gfx, llvm, linux-kernel,
Andi Shyti
On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>
Hi Nick,
> Thanks, I can repro now.
>
> I haven't detangled the macro soup, but I noticed:
>
> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
> following comment:
> 18 * Mask must be a compilation time constant.
I had comments about this here:
https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
The relevant part being:
---- {quote} ----
> > > ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> > > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
also occurs here):
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
So it goes through previous checks including the "mask is not constant"
check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
after most optimizations have run" so by that time both compilers (gcc and
clang) have figured out that even though _mask is coming in as function
argument it is really the constant below:
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
But it is not clear why clang chokes on this "type of reg too small for
mask" check (and gcc doesn't) since everything is u32.
---- {end quote} ----
>
> 2. hwm_field_scale_and_write only has one callsite.
>
> The following patch works:
If we need to fix it at our end yes we can come up with one of these
patches. But we were hoping someone from clang/llvm can comment about the
"type of reg too small for mask" stuff. If this is something which needs to
be fixed in clang/llvm we probably don't want to hide the issue.
>
> ```
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 9e9781493025..6ac29d90b92a 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
>
> static void
> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> - u32 field_msk, int nshift,
> + int nshift,
> unsigned int scale_factor, long lval)
> {
> u32 nval;
> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
> *ddat, i915_reg_t rgadr,
> /* Computation in 64-bits to avoid overflow. Round to nearest. */
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> - bits_to_clear = field_msk;
> - bits_to_set = FIELD_PREP(field_msk, nval);
> + bits_to_clear = PKG_PWR_LIM_1;
> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> bits_to_clear, bits_to_set);
> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
> attr, int chan, long val)
> case hwmon_power_max:
> hwm_field_scale_and_write(ddat,
> hwmon->rg.pkg_rapl_limit,
> - PKG_PWR_LIM_1,
> hwmon->scl_shift_power,
> SF_POWER, val);
> return 0;
> ```
> Though I'm not sure if you're planning to add further callsites of
> hwm_field_scale_and_write with different field_masks?
I have reasons for keeping it this way, it's there in the link above if you
are interested.
>
> Alternatively, (without the above diff),
>
> ```
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index c9be1657f03d..6f40f12bcf89 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
> #define _LINUX_BITFIELD_H
>
> #include <linux/build_bug.h>
> +#include <linux/const.h>
> #include <asm/byteorder.h>
>
> /*
> @@ -62,7 +63,7 @@
>
> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> ({ \
> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
> _pfx "mask is not constant"); \
> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> ```
> will produce:
> error: call to __compiletime_assert_407 declared with 'error'
> attribute: FIELD_PREP: mask is not constant
>
> I haven't tested if that change is also feasible (on top of fixing
> this specific instance), but I think it might help avoid more of these
> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
> compiler version, optimization level.
Not disagreeing, can do something here if needed.
Thanks.
--
Ashutosh
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-27 18:32 ` Dixit, Ashutosh
@ 2022-10-28 6:26 ` Gwan-gyeong Mun
2022-10-28 6:43 ` Gwan-gyeong Mun
1 sibling, 0 replies; 12+ messages in thread
From: Gwan-gyeong Mun @ 2022-10-28 6:26 UTC (permalink / raw)
To: Dixit, Ashutosh, Nick Desaulniers
Cc: anshuman.gupta, intel-gfx, llvm, linux-kernel, Andi Shyti
Hi all,
I should have written the commit message more accurately, but it seems
that it was written inaccurately.
If the FIELD_PREP macro is expanded, the following macros are used.
#define FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant"); \
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
Among them, a build error is generated by the lower part of the
__BF_FIELD_CHECK() macro.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
Here, if you apply an argument to this macro, it will look like the
following.
__bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
The result is always false because an unsigned int value of type
field_msk is not always greater than the maximum value of unsigned long
long .
So, a build error occurs due to the following part of the clang compiler
option.
[-Werror,-Wtautological-constant-out-of-range-compare]
You can simply override this warning in Clang by adding the build option
below, but this seems like a bad attempt
i915/Makefile
CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
The easiest way to solve this is to use a constant value, not a
variable, as an argument to FIELD_PREP.
And since the REG_FIELD_PREP() macro suggested by Jani requires a const
expression as the first argument, it cannot be changed with this macro
alone in the existing code, it must be changed to input a constant value
as shown below.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 08c921421a5f..abb3a194c548 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
static void
hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- const u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
/* Computation in 64-bits to avoid overflow. Round to nearest. */
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
- bits_to_clear = field_msk;
- bits_to_set = REG_FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
int chan, long val)
case hwmon_power_max:
hwm_field_scale_and_write(ddat,
hwmon->rg.pkg_rapl_limit,
- PKG_PWR_LIM_1,
hwmon->scl_shift_power,
SF_POWER, val);
return 0;
In addition, if there is no build problem regardless of the size of the
type as the first argument in FIELD_PREP, it is possible through the
following modification.
(Since this modification modifies include/linux/bitfield.h , I will send
it as a separate patch.
)
However, it seems that we need to have Jani's confirm whether it is okay
to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
return type.
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03d..6e96799b6f38 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -9,7 +9,7 @@
#include <linux/build_bug.h>
#include <asm/byteorder.h>
-
+#include <linux/overflow.h>
/*
* Bitfield access macros
*
@@ -69,7 +69,7 @@
~((_mask) >> __bf_shf(_mask)) & (_val)
: 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
- __bf_cast_unsigned(_reg, ~0ull), \
+ __bf_cast_unsigned(_reg,
type_max(__unsigned_scalar_typeof(_reg))), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
@@ -84,7 +84,7 @@
*/
#define FIELD_MAX(_mask) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)),
type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \
(typeof(_mask))((_mask) >> __bf_shf(_mask)); \
})
@@ -97,7 +97,7 @@
*/
#define FIELD_FIT(_mask, _val) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)),
type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \
!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
})
@@ -111,7 +111,7 @@
*/
#define FIELD_PREP(_mask, _val)
\
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
@@ -125,7 +125,7 @@
*/
#define FIELD_GET(_mask, _reg) \
({ \
- __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
+ __BF_FIELD_CHECK(_mask, _reg,
type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
Br,
G.G.
On 10/27/22 9:32 PM, Dixit, Ashutosh wrote:
> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>>
>
> Hi Nick,
>
>> Thanks, I can repro now.
>>
>> I haven't detangled the macro soup, but I noticed:
>>
>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
>> following comment:
>> 18 * Mask must be a compilation time constant.
>
> I had comments about this here:
>
> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
>
> The relevant part being:
>
> ---- {quote} ----
>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>
> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
> also occurs here):
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull), \
> _pfx "type of reg too small for mask"); \
>
> So it goes through previous checks including the "mask is not constant"
> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
> after most optimizations have run" so by that time both compilers (gcc and
> clang) have figured out that even though _mask is coming in as function
> argument it is really the constant below:
>
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>
> But it is not clear why clang chokes on this "type of reg too small for
> mask" check (and gcc doesn't) since everything is u32.
> ---- {end quote} ----
>
>>
>> 2. hwm_field_scale_and_write only has one callsite.
>>
>> The following patch works:
>
> If we need to fix it at our end yes we can come up with one of these
> patches. But we were hoping someone from clang/llvm can comment about the
> "type of reg too small for mask" stuff. If this is something which needs to
> be fixed in clang/llvm we probably don't want to hide the issue.
>
>>
>> ```
>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>> b/drivers/gpu/drm/i915/i915_hwmon.c
>> index 9e9781493025..6ac29d90b92a 100644
>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>> i915_reg_t rgadr,
>>
>> static void
>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>> - u32 field_msk, int nshift,
>> + int nshift,
>> unsigned int scale_factor, long lval)
>> {
>> u32 nval;
>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
>> *ddat, i915_reg_t rgadr,
>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>
>> - bits_to_clear = field_msk;
>> - bits_to_set = FIELD_PREP(field_msk, nval);
>> + bits_to_clear = PKG_PWR_LIM_1;
>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>>
>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> bits_to_clear, bits_to_set);
>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
>> attr, int chan, long val)
>> case hwmon_power_max:
>> hwm_field_scale_and_write(ddat,
>> hwmon->rg.pkg_rapl_limit,
>> - PKG_PWR_LIM_1,
>> hwmon->scl_shift_power,
>> SF_POWER, val);
>> return 0;
>> ```
>> Though I'm not sure if you're planning to add further callsites of
>> hwm_field_scale_and_write with different field_masks?
>
> I have reasons for keeping it this way, it's there in the link above if you
> are interested.
>
>>
>> Alternatively, (without the above diff),
>>
>> ```
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index c9be1657f03d..6f40f12bcf89 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -8,6 +8,7 @@
>> #define _LINUX_BITFIELD_H
>>
>> #include <linux/build_bug.h>
>> +#include <linux/const.h>
>> #include <asm/byteorder.h>
>>
>> /*
>> @@ -62,7 +63,7 @@
>>
>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
>> ({ \
>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
>> _pfx "mask is not constant"); \
>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>> ```
>> will produce:
>> error: call to __compiletime_assert_407 declared with 'error'
>> attribute: FIELD_PREP: mask is not constant
>>
>> I haven't tested if that change is also feasible (on top of fixing
>> this specific instance), but I think it might help avoid more of these
>> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
>> compiler version, optimization level.
>
> Not disagreeing, can do something here if needed.
>
> Thanks.
> --
> Ashutosh
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-27 18:32 ` Dixit, Ashutosh
2022-10-28 6:26 ` Gwan-gyeong Mun
@ 2022-10-28 6:43 ` Gwan-gyeong Mun
2022-10-28 8:46 ` Jani Nikula
1 sibling, 1 reply; 12+ messages in thread
From: Gwan-gyeong Mun @ 2022-10-28 6:43 UTC (permalink / raw)
To: Dixit, Ashutosh, Nick Desaulniers
Cc: anshuman.gupta, intel-gfx, llvm, linux-kernel, Andi Shyti, Jani Nikula
Resend, because some content was accidentally omitted from the previous
reply.
Please ignore the previous email.
Hi all,
I should have written the original commit message more accurately, but
it seems that it was written inaccurately.
If the FIELD_PREP macro is expanded, the following macros are used.
#define FIELD_PREP(_mask, _val) \
({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
({ \
BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
_pfx "mask is not constant"); \
BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
})
Among them, a build error is generated by the lower part of the
__BF_FIELD_CHECK() macro.
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
__bf_cast_unsigned(_reg, ~0ull), \
_pfx "type of reg too small for mask"); \
Here, if you apply an argument to this macro, it will look like the
following.
__bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
The result is always false because an unsigned int value of type
field_msk is not always greater than the maximum value of unsigned long
long .
So, a build error occurs due to the following part of the clang compiler
option.
[-Werror,-Wtautological-constant-out-of-range-compare]
You can simply override this warning in Clang by adding the build option
below, but this seems like a bad attempt
i915/Makefile
CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
The easiest way to solve this is to use a constant value, not a
variable, as an argument to FIELD_PREP.
And since the REG_FIELD_PREP() macro suggested by Jani requires a const
expression as the first argument, it cannot be changed with this macro
alone in the existing code, it must be changed to input a constant value
as shown below.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
b/drivers/gpu/drm/i915/i915_hwmon.c
index 08c921421a5f..abb3a194c548 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
static void
hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
- const u32 field_msk, int nshift,
+ int nshift,
unsigned int scale_factor, long lval)
{
u32 nval;
@@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
i915_reg_t rgadr,
/* Computation in 64-bits to avoid overflow. Round to nearest. */
nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
- bits_to_clear = field_msk;
- bits_to_set = REG_FIELD_PREP(field_msk, nval);
+ bits_to_clear = PKG_PWR_LIM_1;
+ bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
bits_to_clear, bits_to_set);
@@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
int chan, long val)
case hwmon_power_max:
hwm_field_scale_and_write(ddat,
hwmon->rg.pkg_rapl_limit,
- PKG_PWR_LIM_1,
hwmon->scl_shift_power,
SF_POWER, val);
return 0;
In addition, if there is no build problem regardless of the size of the
type as the first argument in FIELD_PREP, it is possible through the
following modification.
(Since this modification modifies include/linux/bitfield.h , I will send
it as a separate patch.
)
However, it seems that we need to have Jani's confirm whether it is okay
to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
return type in i915.
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index c9be1657f03d..6e96799b6f38 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -9,7 +9,7 @@
#include <linux/build_bug.h>
#include <asm/byteorder.h>
-
+#include <linux/overflow.h>
/*
* Bitfield access macros
*
@@ -69,7 +69,7 @@
~((_mask) >> __bf_shf(_mask)) & (_val)
: 0, \
_pfx "value too large for the field"); \
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
- __bf_cast_unsigned(_reg, ~0ull), \
+ __bf_cast_unsigned(_reg,
type_max(__unsigned_scalar_typeof(_reg))), \
_pfx "type of reg too small for mask"); \
__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
(1ULL << __bf_shf(_mask))); \
@@ -84,7 +84,7 @@
*/
#define FIELD_MAX(_mask) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)),
type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \
(typeof(_mask))((_mask) >> __bf_shf(_mask)); \
})
@@ -97,7 +97,7 @@
*/
#define FIELD_FIT(_mask, _val) \
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)),
type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \
!((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
})
@@ -111,7 +111,7 @@
*/
#define FIELD_PREP(_mask, _val)
\
({ \
- __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
+ __BF_FIELD_CHECK(_mask,
type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \
((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
})
@@ -125,7 +125,7 @@
*/
#define FIELD_GET(_mask, _reg) \
({ \
- __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
+ __BF_FIELD_CHECK(_mask, _reg,
type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \
(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
})
Br,
G.G.
On 10/27/22 9:32 PM, Dixit, Ashutosh wrote:
> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>>
>
> Hi Nick,
>
>> Thanks, I can repro now.
>>
>> I haven't detangled the macro soup, but I noticed:
>>
>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
>> following comment:
>> 18 * Mask must be a compilation time constant.
>
> I had comments about this here:
>
> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
>
> The relevant part being:
>
> ---- {quote} ----
>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>
> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
> also occurs here):
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull), \
> _pfx "type of reg too small for mask"); \
>
> So it goes through previous checks including the "mask is not constant"
> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
> after most optimizations have run" so by that time both compilers (gcc and
> clang) have figured out that even though _mask is coming in as function
> argument it is really the constant below:
>
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>
> But it is not clear why clang chokes on this "type of reg too small for
> mask" check (and gcc doesn't) since everything is u32.
> ---- {end quote} ----
>
>>
>> 2. hwm_field_scale_and_write only has one callsite.
>>
>> The following patch works:
>
> If we need to fix it at our end yes we can come up with one of these
> patches. But we were hoping someone from clang/llvm can comment about the
> "type of reg too small for mask" stuff. If this is something which needs to
> be fixed in clang/llvm we probably don't want to hide the issue.
>
>>
>> ```
>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>> b/drivers/gpu/drm/i915/i915_hwmon.c
>> index 9e9781493025..6ac29d90b92a 100644
>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>> i915_reg_t rgadr,
>>
>> static void
>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>> - u32 field_msk, int nshift,
>> + int nshift,
>> unsigned int scale_factor, long lval)
>> {
>> u32 nval;
>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
>> *ddat, i915_reg_t rgadr,
>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>
>> - bits_to_clear = field_msk;
>> - bits_to_set = FIELD_PREP(field_msk, nval);
>> + bits_to_clear = PKG_PWR_LIM_1;
>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>>
>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>> bits_to_clear, bits_to_set);
>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
>> attr, int chan, long val)
>> case hwmon_power_max:
>> hwm_field_scale_and_write(ddat,
>> hwmon->rg.pkg_rapl_limit,
>> - PKG_PWR_LIM_1,
>> hwmon->scl_shift_power,
>> SF_POWER, val);
>> return 0;
>> ```
>> Though I'm not sure if you're planning to add further callsites of
>> hwm_field_scale_and_write with different field_masks?
>
> I have reasons for keeping it this way, it's there in the link above if you
> are interested.
>
>>
>> Alternatively, (without the above diff),
>>
>> ```
>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>> index c9be1657f03d..6f40f12bcf89 100644
>> --- a/include/linux/bitfield.h
>> +++ b/include/linux/bitfield.h
>> @@ -8,6 +8,7 @@
>> #define _LINUX_BITFIELD_H
>>
>> #include <linux/build_bug.h>
>> +#include <linux/const.h>
>> #include <asm/byteorder.h>
>>
>> /*
>> @@ -62,7 +63,7 @@
>>
>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
>> ({ \
>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
>> _pfx "mask is not constant"); \
>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>> ```
>> will produce:
>> error: call to __compiletime_assert_407 declared with 'error'
>> attribute: FIELD_PREP: mask is not constant
>>
>> I haven't tested if that change is also feasible (on top of fixing
>> this specific instance), but I think it might help avoid more of these
>> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
>> compiler version, optimization level.
>
> Not disagreeing, can do something here if needed.
>
> Thanks.
> --
> Ashutosh
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-28 6:43 ` Gwan-gyeong Mun
@ 2022-10-28 8:46 ` Jani Nikula
2022-11-02 6:32 ` [Intel-gfx] " Joonas Lahtinen
0 siblings, 1 reply; 12+ messages in thread
From: Jani Nikula @ 2022-10-28 8:46 UTC (permalink / raw)
To: Gwan-gyeong Mun, Dixit, Ashutosh, Nick Desaulniers
Cc: anshuman.gupta, intel-gfx, llvm, linux-kernel, Andi Shyti
On Fri, 28 Oct 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
> Resend, because some content was accidentally omitted from the previous
> reply.
> Please ignore the previous email.
>
> Hi all,
>
> I should have written the original commit message more accurately, but
> it seems that it was written inaccurately.
>
> If the FIELD_PREP macro is expanded, the following macros are used.
>
> #define FIELD_PREP(_mask, _val) \
> ({ \
> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> })
>
>
> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> ({ \
> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> _pfx "mask is not constant"); \
> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
> _pfx "value too large for the field"); \
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull), \
> _pfx "type of reg too small for mask"); \
> __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> (1ULL << __bf_shf(_mask))); \
> })
>
> Among them, a build error is generated by the lower part of the
> __BF_FIELD_CHECK() macro.
>
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> __bf_cast_unsigned(_reg, ~0ull), \
> _pfx "type of reg too small for mask"); \
>
>
> Here, if you apply an argument to this macro, it will look like the
> following.
>
> __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
>
> The result is always false because an unsigned int value of type
> field_msk is not always greater than the maximum value of unsigned long
> long .
> So, a build error occurs due to the following part of the clang compiler
> option.
>
> [-Werror,-Wtautological-constant-out-of-range-compare]
>
> You can simply override this warning in Clang by adding the build option
> below, but this seems like a bad attempt
>
> i915/Makefile
> CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
>
> The easiest way to solve this is to use a constant value, not a
> variable, as an argument to FIELD_PREP.
>
> And since the REG_FIELD_PREP() macro suggested by Jani requires a const
> expression as the first argument, it cannot be changed with this macro
> alone in the existing code, it must be changed to input a constant value
> as shown below.
We've added REG_FIELD_PREP() precisely to avoid the problems with the
types and ranges, as we want it to operate on u32. It also uses
__is_constexpr() to avoid dependencies on compiler implementation and
optimizations.
Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
interface if needed.
BR,
Jani.
>
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> b/drivers/gpu/drm/i915/i915_hwmon.c
> index 08c921421a5f..abb3a194c548 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
>
> static void
> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> - const u32 field_msk, int nshift,
> + int nshift,
> unsigned int scale_factor, long lval)
> {
> u32 nval;
> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
> i915_reg_t rgadr,
> /* Computation in 64-bits to avoid overflow. Round to nearest. */
> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>
> - bits_to_clear = field_msk;
> - bits_to_set = REG_FIELD_PREP(field_msk, nval);
> + bits_to_clear = PKG_PWR_LIM_1;
> + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
>
> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> bits_to_clear, bits_to_set);
> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
> int chan, long val)
> case hwmon_power_max:
> hwm_field_scale_and_write(ddat,
> hwmon->rg.pkg_rapl_limit,
> - PKG_PWR_LIM_1,
> hwmon->scl_shift_power,
> SF_POWER, val);
> return 0;
>
>
>
> In addition, if there is no build problem regardless of the size of the
> type as the first argument in FIELD_PREP, it is possible through the
> following modification.
> (Since this modification modifies include/linux/bitfield.h , I will send
> it as a separate patch.
> )
>
> However, it seems that we need to have Jani's confirm whether it is okay
> to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
> return type in i915.
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index c9be1657f03d..6e96799b6f38 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -9,7 +9,7 @@
>
> #include <linux/build_bug.h>
> #include <asm/byteorder.h>
> -
> +#include <linux/overflow.h>
> /*
> * Bitfield access macros
> *
> @@ -69,7 +69,7 @@
> ~((_mask) >> __bf_shf(_mask)) & (_val)
> : 0, \
> _pfx "value too large for the field"); \
> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> - __bf_cast_unsigned(_reg, ~0ull), \
> + __bf_cast_unsigned(_reg,
> type_max(__unsigned_scalar_typeof(_reg))), \
> _pfx "type of reg too small for mask"); \
> __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> (1ULL << __bf_shf(_mask))); \
> @@ -84,7 +84,7 @@
> */
> #define FIELD_MAX(_mask) \
> ({ \
> - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \
> + __BF_FIELD_CHECK(_mask,
> type_min(__unsigned_scalar_typeof(_mask)),
> type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \
> (typeof(_mask))((_mask) >> __bf_shf(_mask)); \
> })
>
> @@ -97,7 +97,7 @@
> */
> #define FIELD_FIT(_mask, _val) \
> ({ \
> - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
> + __BF_FIELD_CHECK(_mask,
> type_min(__unsigned_scalar_typeof(_mask)),
> type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \
> !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
> })
>
> @@ -111,7 +111,7 @@
> */
> #define FIELD_PREP(_mask, _val)
> \
> ({ \
> - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> + __BF_FIELD_CHECK(_mask,
> type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \
> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> })
>
> @@ -125,7 +125,7 @@
> */
> #define FIELD_GET(_mask, _reg) \
> ({ \
> - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
> + __BF_FIELD_CHECK(_mask, _reg,
> type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \
> (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> })
>
>
> Br,
>
> G.G.
>
> On 10/27/22 9:32 PM, Dixit, Ashutosh wrote:
>> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>>>
>>
>> Hi Nick,
>>
>>> Thanks, I can repro now.
>>>
>>> I haven't detangled the macro soup, but I noticed:
>>>
>>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
>>> following comment:
>>> 18 * Mask must be a compilation time constant.
>>
>> I had comments about this here:
>>
>> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
>>
>> The relevant part being:
>>
>> ---- {quote} ----
>>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
>>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>
>> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
>> also occurs here):
>>
>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>> __bf_cast_unsigned(_reg, ~0ull), \
>> _pfx "type of reg too small for mask"); \
>>
>> So it goes through previous checks including the "mask is not constant"
>> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
>> after most optimizations have run" so by that time both compilers (gcc and
>> clang) have figured out that even though _mask is coming in as function
>> argument it is really the constant below:
>>
>> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>>
>> But it is not clear why clang chokes on this "type of reg too small for
>> mask" check (and gcc doesn't) since everything is u32.
>> ---- {end quote} ----
>>
>>>
>>> 2. hwm_field_scale_and_write only has one callsite.
>>>
>>> The following patch works:
>>
>> If we need to fix it at our end yes we can come up with one of these
>> patches. But we were hoping someone from clang/llvm can comment about the
>> "type of reg too small for mask" stuff. If this is something which needs to
>> be fixed in clang/llvm we probably don't want to hide the issue.
>>
>>>
>>> ```
>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>>> b/drivers/gpu/drm/i915/i915_hwmon.c
>>> index 9e9781493025..6ac29d90b92a 100644
>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>>> i915_reg_t rgadr,
>>>
>>> static void
>>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>>> - u32 field_msk, int nshift,
>>> + int nshift,
>>> unsigned int scale_factor, long lval)
>>> {
>>> u32 nval;
>>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
>>> *ddat, i915_reg_t rgadr,
>>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>>
>>> - bits_to_clear = field_msk;
>>> - bits_to_set = FIELD_PREP(field_msk, nval);
>>> + bits_to_clear = PKG_PWR_LIM_1;
>>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>>>
>>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>>> bits_to_clear, bits_to_set);
>>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
>>> attr, int chan, long val)
>>> case hwmon_power_max:
>>> hwm_field_scale_and_write(ddat,
>>> hwmon->rg.pkg_rapl_limit,
>>> - PKG_PWR_LIM_1,
>>> hwmon->scl_shift_power,
>>> SF_POWER, val);
>>> return 0;
>>> ```
>>> Though I'm not sure if you're planning to add further callsites of
>>> hwm_field_scale_and_write with different field_masks?
>>
>> I have reasons for keeping it this way, it's there in the link above if you
>> are interested.
>>
>>>
>>> Alternatively, (without the above diff),
>>>
>>> ```
>>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>>> index c9be1657f03d..6f40f12bcf89 100644
>>> --- a/include/linux/bitfield.h
>>> +++ b/include/linux/bitfield.h
>>> @@ -8,6 +8,7 @@
>>> #define _LINUX_BITFIELD_H
>>>
>>> #include <linux/build_bug.h>
>>> +#include <linux/const.h>
>>> #include <asm/byteorder.h>
>>>
>>> /*
>>> @@ -62,7 +63,7 @@
>>>
>>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
>>> ({ \
>>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
>>> _pfx "mask is not constant"); \
>>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
>>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>>> ```
>>> will produce:
>>> error: call to __compiletime_assert_407 declared with 'error'
>>> attribute: FIELD_PREP: mask is not constant
>>>
>>> I haven't tested if that change is also feasible (on top of fixing
>>> this specific instance), but I think it might help avoid more of these
>>> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
>>> compiler version, optimization level.
>>
>> Not disagreeing, can do something here if needed.
>>
>> Thanks.
>> --
>> Ashutosh
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-10-28 8:46 ` Jani Nikula
@ 2022-11-02 6:32 ` Joonas Lahtinen
2022-11-02 10:41 ` Gwan-gyeong Mun
0 siblings, 1 reply; 12+ messages in thread
From: Joonas Lahtinen @ 2022-11-02 6:32 UTC (permalink / raw)
To: Dixit, Ashutosh, Gwan-gyeong Mun, Jani Nikula, Nick Desaulniers
Cc: intel-gfx, llvm, linux-kernel
Quoting Jani Nikula (2022-10-28 11:46:21)
> On Fri, 28 Oct 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
> > Resend, because some content was accidentally omitted from the previous
> > reply.
> > Please ignore the previous email.
> >
> > Hi all,
> >
> > I should have written the original commit message more accurately, but
> > it seems that it was written inaccurately.
> >
> > If the FIELD_PREP macro is expanded, the following macros are used.
> >
> > #define FIELD_PREP(_mask, _val) \
> > ({ \
> > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> > })
> >
> >
> > #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> > ({ \
> > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> > _pfx "mask is not constant"); \
> > BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
> > _pfx "value too large for the field"); \
> > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> > __bf_cast_unsigned(_reg, ~0ull), \
> > _pfx "type of reg too small for mask"); \
> > __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> > (1ULL << __bf_shf(_mask))); \
> > })
> >
> > Among them, a build error is generated by the lower part of the
> > __BF_FIELD_CHECK() macro.
> >
> > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> > __bf_cast_unsigned(_reg, ~0ull), \
> > _pfx "type of reg too small for mask"); \
> >
> >
> > Here, if you apply an argument to this macro, it will look like the
> > following.
> >
> > __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
> >
> > The result is always false because an unsigned int value of type
> > field_msk is not always greater than the maximum value of unsigned long
> > long .
> > So, a build error occurs due to the following part of the clang compiler
> > option.
> >
> > [-Werror,-Wtautological-constant-out-of-range-compare]
> >
> > You can simply override this warning in Clang by adding the build option
> > below, but this seems like a bad attempt
> >
> > i915/Makefile
> > CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
> >
> > The easiest way to solve this is to use a constant value, not a
> > variable, as an argument to FIELD_PREP.
> >
> > And since the REG_FIELD_PREP() macro suggested by Jani requires a const
> > expression as the first argument, it cannot be changed with this macro
> > alone in the existing code, it must be changed to input a constant value
> > as shown below.
>
> We've added REG_FIELD_PREP() precisely to avoid the problems with the
> types and ranges, as we want it to operate on u32. It also uses
> __is_constexpr() to avoid dependencies on compiler implementation and
> optimizations.
>
> Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
> interface if needed.
Ashutosh and GG, can we get a fix for this merged ASAP. It's currently
blocking the drm-intel-gt-next pull request.
Regards, Joonas
>
> BR,
> Jani.
>
>
>
>
> >
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> > b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 08c921421a5f..abb3a194c548 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> > i915_reg_t rgadr,
> >
> > static void
> > hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> > - const u32 field_msk, int nshift,
> > + int nshift,
> > unsigned int scale_factor, long lval)
> > {
> > u32 nval;
> > @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
> > i915_reg_t rgadr,
> > /* Computation in 64-bits to avoid overflow. Round to nearest. */
> > nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> >
> > - bits_to_clear = field_msk;
> > - bits_to_set = REG_FIELD_PREP(field_msk, nval);
> > + bits_to_clear = PKG_PWR_LIM_1;
> > + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
> >
> > hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> > bits_to_clear, bits_to_set);
> > @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
> > int chan, long val)
> > case hwmon_power_max:
> > hwm_field_scale_and_write(ddat,
> > hwmon->rg.pkg_rapl_limit,
> > - PKG_PWR_LIM_1,
> > hwmon->scl_shift_power,
> > SF_POWER, val);
> > return 0;
> >
> >
> >
> > In addition, if there is no build problem regardless of the size of the
> > type as the first argument in FIELD_PREP, it is possible through the
> > following modification.
> > (Since this modification modifies include/linux/bitfield.h , I will send
> > it as a separate patch.
> > )
> >
> > However, it seems that we need to have Jani's confirm whether it is okay
> > to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
> > return type in i915.
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index c9be1657f03d..6e96799b6f38 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -9,7 +9,7 @@
> >
> > #include <linux/build_bug.h>
> > #include <asm/byteorder.h>
> > -
> > +#include <linux/overflow.h>
> > /*
> > * Bitfield access macros
> > *
> > @@ -69,7 +69,7 @@
> > ~((_mask) >> __bf_shf(_mask)) & (_val)
> > : 0, \
> > _pfx "value too large for the field"); \
> > BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> > - __bf_cast_unsigned(_reg, ~0ull), \
> > + __bf_cast_unsigned(_reg,
> > type_max(__unsigned_scalar_typeof(_reg))), \
> > _pfx "type of reg too small for mask"); \
> > __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
> > (1ULL << __bf_shf(_mask))); \
> > @@ -84,7 +84,7 @@
> > */
> > #define FIELD_MAX(_mask) \
> > ({ \
> > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \
> > + __BF_FIELD_CHECK(_mask,
> > type_min(__unsigned_scalar_typeof(_mask)),
> > type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \
> > (typeof(_mask))((_mask) >> __bf_shf(_mask)); \
> > })
> >
> > @@ -97,7 +97,7 @@
> > */
> > #define FIELD_FIT(_mask, _val) \
> > ({ \
> > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
> > + __BF_FIELD_CHECK(_mask,
> > type_min(__unsigned_scalar_typeof(_mask)),
> > type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \
> > !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
> > })
> >
> > @@ -111,7 +111,7 @@
> > */
> > #define FIELD_PREP(_mask, _val)
> > \
> > ({ \
> > - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > + __BF_FIELD_CHECK(_mask,
> > type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \
> > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
> > })
> >
> > @@ -125,7 +125,7 @@
> > */
> > #define FIELD_GET(_mask, _reg) \
> > ({ \
> > - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
> > + __BF_FIELD_CHECK(_mask, _reg,
> > type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \
> > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
> > })
> >
> >
> > Br,
> >
> > G.G.
> >
> > On 10/27/22 9:32 PM, Dixit, Ashutosh wrote:
> >> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
> >>>
> >>
> >> Hi Nick,
> >>
> >>> Thanks, I can repro now.
> >>>
> >>> I haven't detangled the macro soup, but I noticed:
> >>>
> >>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
> >>> following comment:
> >>> 18 * Mask must be a compilation time constant.
> >>
> >> I had comments about this here:
> >>
> >> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
> >>
> >> The relevant part being:
> >>
> >> ---- {quote} ----
> >>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
> >>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> >>
> >> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
> >> also occurs here):
> >>
> >> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
> >> __bf_cast_unsigned(_reg, ~0ull), \
> >> _pfx "type of reg too small for mask"); \
> >>
> >> So it goes through previous checks including the "mask is not constant"
> >> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
> >> after most optimizations have run" so by that time both compilers (gcc and
> >> clang) have figured out that even though _mask is coming in as function
> >> argument it is really the constant below:
> >>
> >> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> >>
> >> But it is not clear why clang chokes on this "type of reg too small for
> >> mask" check (and gcc doesn't) since everything is u32.
> >> ---- {end quote} ----
> >>
> >>>
> >>> 2. hwm_field_scale_and_write only has one callsite.
> >>>
> >>> The following patch works:
> >>
> >> If we need to fix it at our end yes we can come up with one of these
> >> patches. But we were hoping someone from clang/llvm can comment about the
> >> "type of reg too small for mask" stuff. If this is something which needs to
> >> be fixed in clang/llvm we probably don't want to hide the issue.
> >>
> >>>
> >>> ```
> >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
> >>> b/drivers/gpu/drm/i915/i915_hwmon.c
> >>> index 9e9781493025..6ac29d90b92a 100644
> >>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> >>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
> >>> i915_reg_t rgadr,
> >>>
> >>> static void
> >>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
> >>> - u32 field_msk, int nshift,
> >>> + int nshift,
> >>> unsigned int scale_factor, long lval)
> >>> {
> >>> u32 nval;
> >>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
> >>> *ddat, i915_reg_t rgadr,
> >>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
> >>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
> >>>
> >>> - bits_to_clear = field_msk;
> >>> - bits_to_set = FIELD_PREP(field_msk, nval);
> >>> + bits_to_clear = PKG_PWR_LIM_1;
> >>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
> >>>
> >>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
> >>> bits_to_clear, bits_to_set);
> >>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
> >>> attr, int chan, long val)
> >>> case hwmon_power_max:
> >>> hwm_field_scale_and_write(ddat,
> >>> hwmon->rg.pkg_rapl_limit,
> >>> - PKG_PWR_LIM_1,
> >>> hwmon->scl_shift_power,
> >>> SF_POWER, val);
> >>> return 0;
> >>> ```
> >>> Though I'm not sure if you're planning to add further callsites of
> >>> hwm_field_scale_and_write with different field_masks?
> >>
> >> I have reasons for keeping it this way, it's there in the link above if you
> >> are interested.
> >>
> >>>
> >>> Alternatively, (without the above diff),
> >>>
> >>> ```
> >>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> >>> index c9be1657f03d..6f40f12bcf89 100644
> >>> --- a/include/linux/bitfield.h
> >>> +++ b/include/linux/bitfield.h
> >>> @@ -8,6 +8,7 @@
> >>> #define _LINUX_BITFIELD_H
> >>>
> >>> #include <linux/build_bug.h>
> >>> +#include <linux/const.h>
> >>> #include <asm/byteorder.h>
> >>>
> >>> /*
> >>> @@ -62,7 +63,7 @@
> >>>
> >>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> >>> ({ \
> >>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
> >>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
> >>> _pfx "mask is not constant"); \
> >>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
> >>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
> >>> ```
> >>> will produce:
> >>> error: call to __compiletime_assert_407 declared with 'error'
> >>> attribute: FIELD_PREP: mask is not constant
> >>>
> >>> I haven't tested if that change is also feasible (on top of fixing
> >>> this specific instance), but I think it might help avoid more of these
> >>> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
> >>> compiler version, optimization level.
> >>
> >> Not disagreeing, can do something here if needed.
> >>
> >> Thanks.
> >> --
> >> Ashutosh
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler
2022-11-02 6:32 ` [Intel-gfx] " Joonas Lahtinen
@ 2022-11-02 10:41 ` Gwan-gyeong Mun
0 siblings, 0 replies; 12+ messages in thread
From: Gwan-gyeong Mun @ 2022-11-02 10:41 UTC (permalink / raw)
To: Joonas Lahtinen, Dixit, Ashutosh, Jani Nikula, Nick Desaulniers
Cc: intel-gfx, llvm, linux-kernel
On 11/2/22 8:32 AM, Joonas Lahtinen wrote:
> Quoting Jani Nikula (2022-10-28 11:46:21)
>> On Fri, 28 Oct 2022, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote:
>>> Resend, because some content was accidentally omitted from the previous
>>> reply.
>>> Please ignore the previous email.
>>>
>>> Hi all,
>>>
>>> I should have written the original commit message more accurately, but
>>> it seems that it was written inaccurately.
>>>
>>> If the FIELD_PREP macro is expanded, the following macros are used.
>>>
>>> #define FIELD_PREP(_mask, _val) \
>>> ({ \
>>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>>> })
>>>
>>>
>>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
>>> ({ \
>>> BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>>> _pfx "mask is not constant"); \
>>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
>>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>>> ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \
>>> _pfx "value too large for the field"); \
>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>> __bf_cast_unsigned(_reg, ~0ull), \
>>> _pfx "type of reg too small for mask"); \
>>> __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
>>> (1ULL << __bf_shf(_mask))); \
>>> })
>>>
>>> Among them, a build error is generated by the lower part of the
>>> __BF_FIELD_CHECK() macro.
>>>
>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>> __bf_cast_unsigned(_reg, ~0ull), \
>>> _pfx "type of reg too small for mask"); \
>>>
>>>
>>> Here, if you apply an argument to this macro, it will look like the
>>> following.
>>>
>>> __bf_cast_unsigned(field_msk, field_msk) > __bf_cast_unsigned(0ULL, ~0ull)
>>>
>>> The result is always false because an unsigned int value of type
>>> field_msk is not always greater than the maximum value of unsigned long
>>> long .
>>> So, a build error occurs due to the following part of the clang compiler
>>> option.
>>>
>>> [-Werror,-Wtautological-constant-out-of-range-compare]
>>>
>>> You can simply override this warning in Clang by adding the build option
>>> below, but this seems like a bad attempt
>>>
>>> i915/Makefile
>>> CFLAGS_i915_hwmon.o += -Wno-tautological-constant-out-of-range-compare
>>>
>>> The easiest way to solve this is to use a constant value, not a
>>> variable, as an argument to FIELD_PREP.
>>>
>>> And since the REG_FIELD_PREP() macro suggested by Jani requires a const
>>> expression as the first argument, it cannot be changed with this macro
>>> alone in the existing code, it must be changed to input a constant value
>>> as shown below.
>>
>> We've added REG_FIELD_PREP() precisely to avoid the problems with the
>> types and ranges, as we want it to operate on u32. It also uses
>> __is_constexpr() to avoid dependencies on compiler implementation and
>> optimizations.
>>
>> Please use REG_FIELD_PREP() and a constant value. Maybe rethink the
>> interface if needed.
>
> Ashutosh and GG, can we get a fix for this merged ASAP. It's currently
> blocking the drm-intel-gt-next pull request.
>
> Regards, Joonas
>
Hi Joonas,
As a workaround patch, this patch[1] was reviewed by Ashutoshr and acked
by Jani.
[1] https://patchwork.freedesktop.org/patch/509248/?series=110094&rev=5
Br,
G.G.
>>
>> BR,
>> Jani.
>>
>>
>>
>>
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>>> b/drivers/gpu/drm/i915/i915_hwmon.c
>>> index 08c921421a5f..abb3a194c548 100644
>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>>> i915_reg_t rgadr,
>>>
>>> static void
>>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>>> - const u32 field_msk, int nshift,
>>> + int nshift,
>>> unsigned int scale_factor, long lval)
>>> {
>>> u32 nval;
>>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata *ddat,
>>> i915_reg_t rgadr,
>>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>>
>>> - bits_to_clear = field_msk;
>>> - bits_to_set = REG_FIELD_PREP(field_msk, nval);
>>> + bits_to_clear = PKG_PWR_LIM_1;
>>> + bits_to_set = REG_FIELD_PREP(PKG_PWR_LIM_1, nval);
>>>
>>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>>> bits_to_clear, bits_to_set);
>>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr,
>>> int chan, long val)
>>> case hwmon_power_max:
>>> hwm_field_scale_and_write(ddat,
>>> hwmon->rg.pkg_rapl_limit,
>>> - PKG_PWR_LIM_1,
>>> hwmon->scl_shift_power,
>>> SF_POWER, val);
>>> return 0;
>>>
>>>
>>>
>>> In addition, if there is no build problem regardless of the size of the
>>> type as the first argument in FIELD_PREP, it is possible through the
>>> following modification.
>>> (Since this modification modifies include/linux/bitfield.h , I will send
>>> it as a separate patch.
>>> )
>>>
>>> However, it seems that we need to have Jani's confirm whether it is okay
>>> to use FIELD_PREP() instead of REG_FIELD_PREP() which is forced to u32
>>> return type in i915.
>>>
>>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>>> index c9be1657f03d..6e96799b6f38 100644
>>> --- a/include/linux/bitfield.h
>>> +++ b/include/linux/bitfield.h
>>> @@ -9,7 +9,7 @@
>>>
>>> #include <linux/build_bug.h>
>>> #include <asm/byteorder.h>
>>> -
>>> +#include <linux/overflow.h>
>>> /*
>>> * Bitfield access macros
>>> *
>>> @@ -69,7 +69,7 @@
>>> ~((_mask) >> __bf_shf(_mask)) & (_val)
>>> : 0, \
>>> _pfx "value too large for the field"); \
>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>> - __bf_cast_unsigned(_reg, ~0ull), \
>>> + __bf_cast_unsigned(_reg,
>>> type_max(__unsigned_scalar_typeof(_reg))), \
>>> _pfx "type of reg too small for mask"); \
>>> __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \
>>> (1ULL << __bf_shf(_mask))); \
>>> @@ -84,7 +84,7 @@
>>> */
>>> #define FIELD_MAX(_mask) \
>>> ({ \
>>> - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \
>>> + __BF_FIELD_CHECK(_mask,
>>> type_min(__unsigned_scalar_typeof(_mask)),
>>> type_min(__unsigned_scalar_typeof(_mask)), "FIELD_MAX: "); \
>>> (typeof(_mask))((_mask) >> __bf_shf(_mask)); \
>>> })
>>>
>>> @@ -97,7 +97,7 @@
>>> */
>>> #define FIELD_FIT(_mask, _val) \
>>> ({ \
>>> - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \
>>> + __BF_FIELD_CHECK(_mask,
>>> type_min(__unsigned_scalar_typeof(_mask)),
>>> type_min(__unsigned_scalar_typeof(_val)), "FIELD_FIT: "); \
>>> !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \
>>> })
>>>
>>> @@ -111,7 +111,7 @@
>>> */
>>> #define FIELD_PREP(_mask, _val)
>>> \
>>> ({ \
>>> - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>>> + __BF_FIELD_CHECK(_mask,
>>> type_min(__unsigned_scalar_typeof(_mask)), _val, "FIELD_PREP: "); \
>>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>>> })
>>>
>>> @@ -125,7 +125,7 @@
>>> */
>>> #define FIELD_GET(_mask, _reg) \
>>> ({ \
>>> - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \
>>> + __BF_FIELD_CHECK(_mask, _reg,
>>> type_min(__unsigned_scalar_typeof(_reg)), "FIELD_GET: "); \
>>> (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \
>>> })
>>>
>>>
>>> Br,
>>>
>>> G.G.
>>>
>>> On 10/27/22 9:32 PM, Dixit, Ashutosh wrote:
>>>> On Thu, 27 Oct 2022 10:16:47 -0700, Nick Desaulniers wrote:
>>>>>
>>>>
>>>> Hi Nick,
>>>>
>>>>> Thanks, I can repro now.
>>>>>
>>>>> I haven't detangled the macro soup, but I noticed:
>>>>>
>>>>> 1. FIELD_PREP is defined in include/linux/bitfield.h which has the
>>>>> following comment:
>>>>> 18 * Mask must be a compilation time constant.
>>>>
>>>> I had comments about this here:
>>>>
>>>> https://lore.kernel.org/intel-gfx/87ilk7pwrw.wl-ashutosh.dixit@intel.com/
>>>>
>>>> The relevant part being:
>>>>
>>>> ---- {quote} ----
>>>>>>> ./include/linux/bitfield.h:71:53: note: expanded from macro '__BF_FIELD_CHECK'
>>>>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>>>
>>>> So clang seems to break here at this line in __BF_FIELD_CHECK (note ~0ull
>>>> also occurs here):
>>>>
>>>> BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) > \
>>>> __bf_cast_unsigned(_reg, ~0ull), \
>>>> _pfx "type of reg too small for mask"); \
>>>>
>>>> So it goes through previous checks including the "mask is not constant"
>>>> check. As Nick Desaulniers mentions "__builtin_constant_p is evaluated
>>>> after most optimizations have run" so by that time both compilers (gcc and
>>>> clang) have figured out that even though _mask is coming in as function
>>>> argument it is really the constant below:
>>>>
>>>> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>>>>
>>>> But it is not clear why clang chokes on this "type of reg too small for
>>>> mask" check (and gcc doesn't) since everything is u32.
>>>> ---- {end quote} ----
>>>>
>>>>>
>>>>> 2. hwm_field_scale_and_write only has one callsite.
>>>>>
>>>>> The following patch works:
>>>>
>>>> If we need to fix it at our end yes we can come up with one of these
>>>> patches. But we were hoping someone from clang/llvm can comment about the
>>>> "type of reg too small for mask" stuff. If this is something which needs to
>>>> be fixed in clang/llvm we probably don't want to hide the issue.
>>>>
>>>>>
>>>>> ```
>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c
>>>>> b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>> index 9e9781493025..6ac29d90b92a 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>> @@ -101,7 +101,7 @@ hwm_field_read_and_scale(struct hwm_drvdata *ddat,
>>>>> i915_reg_t rgadr,
>>>>>
>>>>> static void
>>>>> hwm_field_scale_and_write(struct hwm_drvdata *ddat, i915_reg_t rgadr,
>>>>> - u32 field_msk, int nshift,
>>>>> + int nshift,
>>>>> unsigned int scale_factor, long lval)
>>>>> {
>>>>> u32 nval;
>>>>> @@ -111,8 +111,8 @@ hwm_field_scale_and_write(struct hwm_drvdata
>>>>> *ddat, i915_reg_t rgadr,
>>>>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>>>>> nval = DIV_ROUND_CLOSEST_ULL((u64)lval << nshift, scale_factor);
>>>>>
>>>>> - bits_to_clear = field_msk;
>>>>> - bits_to_set = FIELD_PREP(field_msk, nval);
>>>>> + bits_to_clear = PKG_PWR_LIM_1;
>>>>> + bits_to_set = FIELD_PREP(PKG_PWR_LIM_1, nval);
>>>>>
>>>>> hwm_locked_with_pm_intel_uncore_rmw(ddat, rgadr,
>>>>> bits_to_clear, bits_to_set);
>>>>> @@ -406,7 +406,6 @@ hwm_power_write(struct hwm_drvdata *ddat, u32
>>>>> attr, int chan, long val)
>>>>> case hwmon_power_max:
>>>>> hwm_field_scale_and_write(ddat,
>>>>> hwmon->rg.pkg_rapl_limit,
>>>>> - PKG_PWR_LIM_1,
>>>>> hwmon->scl_shift_power,
>>>>> SF_POWER, val);
>>>>> return 0;
>>>>> ```
>>>>> Though I'm not sure if you're planning to add further callsites of
>>>>> hwm_field_scale_and_write with different field_masks?
>>>>
>>>> I have reasons for keeping it this way, it's there in the link above if you
>>>> are interested.
>>>>
>>>>>
>>>>> Alternatively, (without the above diff),
>>>>>
>>>>> ```
>>>>> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
>>>>> index c9be1657f03d..6f40f12bcf89 100644
>>>>> --- a/include/linux/bitfield.h
>>>>> +++ b/include/linux/bitfield.h
>>>>> @@ -8,6 +8,7 @@
>>>>> #define _LINUX_BITFIELD_H
>>>>>
>>>>> #include <linux/build_bug.h>
>>>>> +#include <linux/const.h>
>>>>> #include <asm/byteorder.h>
>>>>>
>>>>> /*
>>>>> @@ -62,7 +63,7 @@
>>>>>
>>>>> #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
>>>>> ({ \
>>>>> - BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \
>>>>> + BUILD_BUG_ON_MSG(!__is_constexpr(_mask), \
>>>>> _pfx "mask is not constant"); \
>>>>> BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \
>>>>> BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \
>>>>> ```
>>>>> will produce:
>>>>> error: call to __compiletime_assert_407 declared with 'error'
>>>>> attribute: FIELD_PREP: mask is not constant
>>>>>
>>>>> I haven't tested if that change is also feasible (on top of fixing
>>>>> this specific instance), but I think it might help avoid more of these
>>>>> subtleties wrt. __builtin_constant_p that depende heavily on compiler,
>>>>> compiler version, optimization level.
>>>>
>>>> Not disagreeing, can do something here if needed.
>>>>
>>>> Thanks.
>>>> --
>>>> Ashutosh
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-11-02 10:42 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20221024210953.1572998-1-gwan-gyeong.mun@intel.com>
[not found] ` <87mt9kppb6.wl-ashutosh.dixit@intel.com>
[not found] ` <Y1ercgaqQwfqt42U@ashyti-mobl2.lan>
2022-10-25 16:46 ` [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Nick Desaulniers
2022-10-25 18:45 ` Dixit, Ashutosh
2022-10-26 0:18 ` Andi Shyti
2022-10-27 16:35 ` Nick Desaulniers
2022-10-27 16:53 ` Dixit, Ashutosh
2022-10-27 17:16 ` Nick Desaulniers
2022-10-27 18:32 ` Dixit, Ashutosh
2022-10-28 6:26 ` Gwan-gyeong Mun
2022-10-28 6:43 ` Gwan-gyeong Mun
2022-10-28 8:46 ` Jani Nikula
2022-11-02 6:32 ` [Intel-gfx] " Joonas Lahtinen
2022-11-02 10:41 ` Gwan-gyeong Mun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).