Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 27 Oct 2022 11:32:54 -0700
Message-ID: <875yg5xgkp.wl-ashutosh.dixit@intel.com>
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
In-Reply-To: <CAKwvOdmVJn8HvfF9WTnOAc+HsdJ4c1Tdck8E7Caky7AoCq4ZTA@mail.gmail.com>
References: <20221024210953.1572998-1-gwan-gyeong.mun@intel.com>        <87mt9kppb6.wl-ashutosh.dixit@intel.com>        <Y1ercgaqQwfqt42U@ashyti-mobl2.lan>     <87ilk7pwrw.wl-ashutosh.dixit@intel.com>        <Y1h8yn4QHI3aBlCe@ashyti-mobl2.lan>     <CAKwvOdkpQvk31zbipLDPXfsDZ8FpGHs9t-+9JfFQO85Bs4h=wg@mail.gmail.com>    <877d0lxl6s.wl-ashutosh.dixit@intel.com>        <CAKwvOdmVJn8HvfF9WTnOAc+HsdJ4c1Tdck8E7Caky7AoCq4ZTA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0
 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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