From: Nick Desaulniers <ndesaulniers@google.com> To: Andi Shyti <andi.shyti@linux.intel.com> Cc: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>, anshuman.gupta@intel.com, intel-gfx@lists.freedesktop.org, llvm@lists.linux.dev, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Date: Tue, 25 Oct 2022 09:46:11 -0700 [thread overview] Message-ID: <CAKwvOdn=O0WrZftj+zzV2iiUgTcXDYdvgJEj5Qfftd92f0aM_w@mail.gmail.com> (raw) In-Reply-To: <Y1ercgaqQwfqt42U@ashyti-mobl2.lan> 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
WARNING: multiple messages have this Message-ID (diff)
From: Nick Desaulniers <ndesaulniers@google.com> To: Andi Shyti <andi.shyti@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, llvm@lists.linux.dev, LKML <linux-kernel@vger.kernel.org> Subject: Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Date: Tue, 25 Oct 2022 09:46:11 -0700 [thread overview] Message-ID: <CAKwvOdn=O0WrZftj+zzV2iiUgTcXDYdvgJEj5Qfftd92f0aM_w@mail.gmail.com> (raw) In-Reply-To: <Y1ercgaqQwfqt42U@ashyti-mobl2.lan> 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
next prev parent reply other threads:[~2022-10-25 16:46 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-10-24 21:09 [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Gwan-gyeong Mun 2022-10-25 1:34 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork 2022-10-25 1:57 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 2022-10-25 3:14 ` [PATCH] " Dixit, Ashutosh 2022-10-25 3:14 ` [Intel-gfx] " Dixit, Ashutosh 2022-10-25 9:25 ` Andi Shyti 2022-10-25 9:25 ` Andi Shyti 2022-10-25 16:46 ` Nick Desaulniers [this message] 2022-10-25 16:46 ` [Intel-gfx] " Nick Desaulniers 2022-10-25 18:45 ` Dixit, Ashutosh 2022-10-25 18:45 ` [Intel-gfx] " Dixit, Ashutosh 2022-10-26 0:18 ` Andi Shyti 2022-10-26 0:18 ` [Intel-gfx] " Andi Shyti 2022-10-27 16:35 ` Nick Desaulniers 2022-10-27 16:35 ` [Intel-gfx] " Nick Desaulniers 2022-10-27 16:53 ` Dixit, Ashutosh 2022-10-27 16:53 ` [Intel-gfx] " Dixit, Ashutosh 2022-10-27 17:16 ` Nick Desaulniers 2022-10-27 17:16 ` Nick Desaulniers 2022-10-27 18:32 ` Dixit, Ashutosh 2022-10-27 18:32 ` [Intel-gfx] " Dixit, Ashutosh 2022-10-28 6:26 ` Gwan-gyeong Mun 2022-10-28 6:26 ` [Intel-gfx] " Gwan-gyeong Mun 2022-10-28 6:43 ` Gwan-gyeong Mun 2022-10-28 6:43 ` Gwan-gyeong Mun 2022-10-28 8:46 ` Jani Nikula 2022-10-28 8:46 ` [Intel-gfx] " Jani Nikula 2022-11-02 6:32 ` Joonas Lahtinen 2022-11-02 10:41 ` Gwan-gyeong Mun 2022-11-02 10:41 ` Gwan-gyeong Mun 2022-10-25 9:15 ` Andi Shyti 2022-10-25 14:27 ` Jani Nikula 2022-10-25 14:30 ` Jani Nikula 2022-10-25 18:46 ` Dixit, Ashutosh 2022-10-27 17:54 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/hwmon: Fix a build error used with clang compiler (rev2) Patchwork 2022-10-28 6:30 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/hwmon: Fix a build error used with clang compiler (rev3) Patchwork 2022-10-28 6:48 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915/hwmon: Fix a build error used with clang compiler (rev4) Patchwork 2022-10-29 4:42 ` [Intel-gfx] [PATCH v2] drm/i915/hwmon: Fix a build error used with clang compiler Gwan-gyeong Mun 2022-10-31 5:19 ` Dixit, Ashutosh 2022-10-31 6:37 ` Gwan-gyeong Mun 2022-10-31 17:31 ` Dixit, Ashutosh 2022-11-01 10:35 ` Jani Nikula 2022-11-02 7:12 ` Dixit, Ashutosh 2022-11-02 8:50 ` Jani Nikula 2022-10-29 5:00 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/hwmon: Fix a build error used with clang compiler (rev5) Patchwork 2022-10-29 5:32 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2022-10-29 15:32 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAKwvOdn=O0WrZftj+zzV2iiUgTcXDYdvgJEj5Qfftd92f0aM_w@mail.gmail.com' \ --to=ndesaulniers@google.com \ --cc=andi.shyti@linux.intel.com \ --cc=anshuman.gupta@intel.com \ --cc=ashutosh.dixit@intel.com \ --cc=gwan-gyeong.mun@intel.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=llvm@lists.linux.dev \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.