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@xxxxxxxxx/ 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
