Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 14:11, David Laight wrote:
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > No idea why the compiler would know that the values are invalid.
> >
> > It's not that the compiler knows tat they are invalid, but I bet what
> > happens is in scale() (and possibly other places that do similar
> > checks), which does this:
> >
> >          WARN_ON(source_min > source_max);
> >          ...
> >          source_val = clamp(source_val, source_min, source_max);
> >
> > and the compiler notices that the ordering comparison in the first
> > WARN_ON() is the same as the one in clamp(), so it basically converts
> > the logic to
> >
> >          if (source_min > source_max) {
> >                  WARN(..);
> >                  /* Do the clamp() knowing that source_min > source_max */
> >                  source_val = clamp(source_val, source_min, source_max);
> >          } else {
> >                  /* Do the clamp knowing that source_min <= source_max */
> >                  source_val = clamp(source_val, source_min, source_max);
> >          }
> >
> > (obviously I dropped the other WARN_ON in the conversion, it wasn't
> > relevant for this case).
> >
> > And now that first clamp() case is done with source_min > source_max,
> > and it triggers that build error because that's invalid.
> >
> > So the condition is not statically true in the *source* code, but in
> > the "I have moved code around to combine tests" case it now *is*
> > statically true as far as the compiler is concerned.
>
> Well spotted :-)
>
> One option would be to move the WARN_ON() below the clamp() and
> add an OPTIMISER_HIDE_VAR(source_max) between them.
>
> Or do something more sensible than the WARN().
> Perhaps return target_min on any such errors?

This helps:

-       WARN_ON(source_min > source_max);
-       WARN_ON(target_min > target_max);
-
        /* defensive */
        source_val = clamp(source_val, source_min, source_max);

+       WARN_ON(source_min > source_max);
+       WARN_ON(target_min > target_max);

Guenter