From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: Nick Desaulniers <ndesaulniers@google.com> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>, anshuman.gupta@intel.com, intel-gfx@lists.freedesktop.org, llvm@lists.linux.dev, linux-kernel@vger.kernel.org, Andi Shyti <andi.shyti@linux.intel.com> Subject: Re: [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Date: Thu, 27 Oct 2022 11:32:54 -0700 [thread overview] Message-ID: <875yg5xgkp.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <CAKwvOdmVJn8HvfF9WTnOAc+HsdJ4c1Tdck8E7Caky7AoCq4ZTA@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com> To: Nick Desaulniers <ndesaulniers@google.com> Cc: intel-gfx@lists.freedesktop.org, llvm@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] drm/i915/hwmon: Fix a build error used with clang compiler Date: Thu, 27 Oct 2022 11:32:54 -0700 [thread overview] Message-ID: <875yg5xgkp.wl-ashutosh.dixit@intel.com> (raw) In-Reply-To: <CAKwvOdmVJn8HvfF9WTnOAc+HsdJ4c1Tdck8E7Caky7AoCq4ZTA@mail.gmail.com> 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
next prev parent reply other threads:[~2022-10-27 18:32 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 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 [this message] 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=875yg5xgkp.wl-ashutosh.dixit@intel.com \ --to=ashutosh.dixit@intel.com \ --cc=andi.shyti@linux.intel.com \ --cc=anshuman.gupta@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 \ --cc=ndesaulniers@google.com \ /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.