* 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).