Re: Buiild error in i915/xe

[Guenter Roeck <linux@roeck-us.net>]

On 1/18/25 14:11, David Laight wrote:
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> 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