A new warning in clang warns that there is an instance where boolean
expressions are being used with bitwise operators instead of logical
ones:
lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
(BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
zstd does this frequently to help with performance, as logical operators
have branches whereas bitwise ones do not.
To fix this warning in other cases, the expressions were placed on
separate lines with the '&=' operator; however, this particular instance
was moved away from that so that it could be surrounded by LIKELY, which
is a macro for __builtin_expect(), to help with a performance
regression, according to upstream zstd pull #1973.
Aside from switching to logical operators, which is likely undesirable
in this instance, or disabling the warning outright, the solution is
casting one of the expressions to an integer type to make it clear to
clang that the author knows what they are doing. Add a cast to U32 to
silence the warning. The first U32 cast is to silence an instance of
-Wshorten-64-to-32 because __builtin_expect() returns long so it cannot
be moved.
Link: https://github.com/ClangBuiltLinux/linux/issues/1486
Link: https://github.com/facebook/zstd/pull/1973
Reported-by: Nick Desaulniers <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
lib/zstd/decompress/huf_decompress.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
index 05570ed5f8be..5105e59ac04a 100644
--- a/lib/zstd/decompress/huf_decompress.c
+++ b/lib/zstd/decompress/huf_decompress.c
@@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
- endSignal = (U32)LIKELY(
+ endSignal = (U32)LIKELY((U32)
(BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
& (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
& (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
--
2.33.1.637.gf443b226ca
> On Oct 21, 2021, at 1:23 PM, Nathan Chancellor <[email protected]> wrote:
>
> A new warning in clang warns that there is an instance where boolean
> expressions are being used with bitwise operators instead of logical
> ones:
>
> lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
> (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> zstd does this frequently to help with performance, as logical operators
> have branches whereas bitwise ones do not.
>
> To fix this warning in other cases, the expressions were placed on
> separate lines with the '&=' operator; however, this particular instance
> was moved away from that so that it could be surrounded by LIKELY, which
> is a macro for __builtin_expect(), to help with a performance
> regression, according to upstream zstd pull #1973.
>
> Aside from switching to logical operators, which is likely undesirable
> in this instance, or disabling the warning outright, the solution is
> casting one of the expressions to an integer type to make it clear to
> clang that the author knows what they are doing. Add a cast to U32 to
> silence the warning. The first U32 cast is to silence an instance of
> -Wshorten-64-to-32 because __builtin_expect() returns long so it cannot
> be moved.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1486
> Link: https://github.com/facebook/zstd/pull/1973
> Reported-by: Nick Desaulniers <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
Thanks for the fix!
I’ll apply this patch to my linux-next tree and port it to upstream zstd.
Best,
Nick Terrell
> ---
> lib/zstd/decompress/huf_decompress.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
> index 05570ed5f8be..5105e59ac04a 100644
> --- a/lib/zstd/decompress/huf_decompress.c
> +++ b/lib/zstd/decompress/huf_decompress.c
> @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
> HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
> HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
> HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
> - endSignal = (U32)LIKELY(
> + endSignal = (U32)LIKELY((U32)
> (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
> & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
> --
> 2.33.1.637.gf443b226ca
>
From: Nick Terrell
> Sent: 26 October 2021 02:18
>
> > On Oct 21, 2021, at 1:23 PM, Nathan Chancellor <[email protected]> wrote:
> >
> > A new warning in clang warns that there is an instance where boolean
> > expressions are being used with bitwise operators instead of logical
> > ones:
> >
> > lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-
> Wbitwise-instead-of-logical]
> > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > zstd does this frequently to help with performance, as logical operators
> > have branches whereas bitwise ones do not.
...
> > The first U32 cast is to silence an instance of -Wshorten-64-to-32
> > because __builtin_expect() returns long so it cannot be moved.
Isn't enabling that warning completely stupid?
The casts required to silence it could easily cause more problems
- by hiding more important bugs. And seriously affect code readability.
...c
> > index 05570ed5f8be..5105e59ac04a 100644
> > --- a/lib/zstd/decompress/huf_decompress.c
> > +++ b/lib/zstd/decompress/huf_decompress.c
> > @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
> > HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
> > HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
> > HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
> > - endSignal = (U32)LIKELY(
> > + endSignal = (U32)LIKELY((U32)
> > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> > & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
> > & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
Isn't that the same as:
((BIT_reload() & BIT_reload() & BIT_reload()) == BIT_DStream_unfinished)
which will generate much better code.
Especially on cpu without 'seteq' instructions.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Oct 26, 2021 at 10:34:31AM +0000, David Laight wrote:
> From: Nick Terrell
> > Sent: 26 October 2021 02:18
> >
> > > On Oct 21, 2021, at 1:23 PM, Nathan Chancellor <[email protected]> wrote:
> > >
> > > A new warning in clang warns that there is an instance where boolean
> > > expressions are being used with bitwise operators instead of logical
> > > ones:
> > >
> > > lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-
> > Wbitwise-instead-of-logical]
> > > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > zstd does this frequently to help with performance, as logical operators
> > > have branches whereas bitwise ones do not.
> ...
> > > The first U32 cast is to silence an instance of -Wshorten-64-to-32
> > > because __builtin_expect() returns long so it cannot be moved.
>
> Isn't enabling that warning completely stupid?
> The casts required to silence it could easily cause more problems
> - by hiding more important bugs. And seriously affect code readability.
Which warning?
-Wbitwise-instead-of-logical is included in clang's -Wall and I do not
think it should be disabled; this is the first instance of the warning
that has been silenced with a cast.
-Wshorten-64-to-32 will never be enabled for Linux but zstd is a
separate project that can be built for a variety of operating systems so
that has to be considered when developing changes for the kernel because
the kernel changes need to go upstream eventually if they touch core
zstd code, otherwise they will just get blown away on the next import.
Specifically, this warning was enabled on iOS:
https://github.com/facebook/zstd/pull/2062
> ...c
> > > index 05570ed5f8be..5105e59ac04a 100644
> > > --- a/lib/zstd/decompress/huf_decompress.c
> > > +++ b/lib/zstd/decompress/huf_decompress.c
> > > @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
> > > HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
> > > HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
> > > HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
> > > - endSignal = (U32)LIKELY(
> > > + endSignal = (U32)LIKELY((U32)
> > > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> > > & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
> > > & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
>
> Isn't that the same as:
> ((BIT_reload() & BIT_reload() & BIT_reload()) == BIT_DStream_unfinished)
> which will generate much better code.
> Especially on cpu without 'seteq' instructions.
I don't think so. Feel free to double check my math.
BIT_reloadDStreamFast() can return either BIT_DStream_unfinished (0) or
BIT_DStream_overflow (3). Let's say the second call returns
BIT_DStream_overflow but the others return BIT_DStream_unfinished.
Current code:
(BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished) &
(BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished) &
(BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
(BIT_DStream_unfinished == BIT_DStream_unfinished) &
(BIT_DStream_overflow == BIT_DStream_unfinished) &
(BIT_DStream_unfinished == BIT_DStream_unfinished)
(1 & 0 & 1)
Final result: 0
Your suggestion:
(BIT_reloadDStreamFast(&bitD1) &
BIT_reloadDStreamFast(&bitD2) &
BIT_reloadDStreamFast(&bitD3)) == BIT_DStream_unfinished
(BIT_DStream_unfinished &
BIT_DStream_overflow &
BIT_DStream_unfinished) == BIT_DStream_unfinished
(0 & 3 & 0) == 0
(0) == 0
Final result: 1
Clang 13.0.0 and GCC 11.2.0 appear agree with me:
https://godbolt.org/z/M78s1TTEx
Cheers,
Nathan
From: Nathan Chancellor
> Sent: 26 October 2021 15:03
...
> > Isn't enabling that warning completely stupid?
> > The casts required to silence it could easily cause more problems
> > - by hiding more important bugs. And seriously affect code readability.
>
> Which warning?
>
> -Wbitwise-instead-of-logical is included in clang's -Wall and I do not
> think it should be disabled; this is the first instance of the warning
> that has been silenced with a cast.
I'm not sure about that one.
I have a feeling it will generate false positives for carefully optimised
code more often that it finds anything where 'short circuiting' will
be a real gain.
Especially for values with are known to be either 0 or 1.
> -Wshorten-64-to-32 will never be enabled for Linux but zstd is a
> separate project that can be built for a variety of operating systems so
> that has to be considered when developing changes for the kernel because
> the kernel changes need to go upstream eventually if they touch core
> zstd code, otherwise they will just get blown away on the next import.
> Specifically, this warning was enabled on iOS:
> https://github.com/facebook/zstd/pull/2062
That one...
If you are going to enable it, then you need a static inline function
to convert u64 to u32, not a C cast.
I'm sure that it won't be long before the compiler writes start an
'open season' on casts.
They really are more dangerous than the warnings they are trying to remove.
> > ...c
> > > > index 05570ed5f8be..5105e59ac04a 100644
> > > > --- a/lib/zstd/decompress/huf_decompress.c
> > > > +++ b/lib/zstd/decompress/huf_decompress.c
> > > > @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
> > > > HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
> > > > HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
> > > > HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
> > > > - endSignal = (U32)LIKELY(
> > > > + endSignal = (U32)LIKELY((U32)
> > > > (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
> > > > & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
> > > > & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
> >
> > Isn't that the same as:
> > ((BIT_reload() & BIT_reload() & BIT_reload()) == BIT_DStream_unfinished)
> > which will generate much better code.
> > Especially on cpu without 'seteq' instructions.
>
> I don't think so. Feel free to double check my math.
>
> BIT_reloadDStreamFast() can return either BIT_DStream_unfinished (0) or
> BIT_DStream_overflow (3)....
Ah, I'd assumed that BIT_DStream_unfinished was non-zero.
So you actually want:
endSignal = !(BIT() | BIT() | BIT());
Just kill the CaMeLs and unnecessary constants.
Then the code becomes succint, easier to read/check etc.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
> On Oct 25, 2021, at 6:17 PM, Nick Terrell <[email protected]> wrote:
>
>
>
>> On Oct 21, 2021, at 1:23 PM, Nathan Chancellor <[email protected]> wrote:
>>
>> A new warning in clang warns that there is an instance where boolean
>> expressions are being used with bitwise operators instead of logical
>> ones:
>>
>> lib/zstd/decompress/huf_decompress.c:890:25: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
>> (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> zstd does this frequently to help with performance, as logical operators
>> have branches whereas bitwise ones do not.
>>
>> To fix this warning in other cases, the expressions were placed on
>> separate lines with the '&=' operator; however, this particular instance
>> was moved away from that so that it could be surrounded by LIKELY, which
>> is a macro for __builtin_expect(), to help with a performance
>> regression, according to upstream zstd pull #1973.
>>
>> Aside from switching to logical operators, which is likely undesirable
>> in this instance, or disabling the warning outright, the solution is
>> casting one of the expressions to an integer type to make it clear to
>> clang that the author knows what they are doing. Add a cast to U32 to
>> silence the warning. The first U32 cast is to silence an instance of
>> -Wshorten-64-to-32 because __builtin_expect() returns long so it cannot
>> be moved.
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1486
>> Link: https://github.com/facebook/zstd/pull/1973
>> Reported-by: Nick Desaulniers <[email protected]>
>> Signed-off-by: Nathan Chancellor <[email protected]>
>
> Thanks for the fix!
>
> I’ll apply this patch to my linux-next tree and port it to upstream zstd.
Applied to my linux-next tree [1] and backported upstream [2].
Thanks again for the patch!
-Nick
[1] https://github.com/terrelln/linux/tree/zstd-1.4.10
[2] https://github.com/facebook/zstd/pull/2849
> Best,
> Nick Terrell
>
>> ---
>> lib/zstd/decompress/huf_decompress.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
>> index 05570ed5f8be..5105e59ac04a 100644
>> --- a/lib/zstd/decompress/huf_decompress.c
>> +++ b/lib/zstd/decompress/huf_decompress.c
>> @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
>> HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
>> HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
>> HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
>> - endSignal = (U32)LIKELY(
>> + endSignal = (U32)LIKELY((U32)
>> (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
>> & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
>> & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
>> --
>> 2.33.1.637.gf443b226ca
> On Oct 26, 2021, at 2:04 PM, David Laight <[email protected]> wrote:
>
> From: Nathan Chancellor
>> Sent: 26 October 2021 15:03
> ...
>>> Isn't enabling that warning completely stupid?
>>> The casts required to silence it could easily cause more problems
>>> - by hiding more important bugs. And seriously affect code readability.
>>
>> Which warning?
>>
>> -Wbitwise-instead-of-logical is included in clang's -Wall and I do not
>> think it should be disabled; this is the first instance of the warning
>> that has been silenced with a cast.
>
> I'm not sure about that one.
> I have a feeling it will generate false positives for carefully optimised
> code more often that it finds anything where 'short circuiting' will
> be a real gain.
> Especially for values with are known to be either 0 or 1.
>
>> -Wshorten-64-to-32 will never be enabled for Linux but zstd is a
>> separate project that can be built for a variety of operating systems so
>> that has to be considered when developing changes for the kernel because
>> the kernel changes need to go upstream eventually if they touch core
>> zstd code, otherwise they will just get blown away on the next import.
>> Specifically, this warning was enabled on iOS:
>> https://github.com/facebook/zstd/pull/2062
>
> That one...
> If you are going to enable it, then you need a static inline function
> to convert u64 to u32, not a C cast.
>
> I'm sure that it won't be long before the compiler writes start an
> 'open season' on casts.
> They really are more dangerous than the warnings they are trying to remove.
>
>>> ...c
>>>>> index 05570ed5f8be..5105e59ac04a 100644
>>>>> --- a/lib/zstd/decompress/huf_decompress.c
>>>>> +++ b/lib/zstd/decompress/huf_decompress.c
>>>>> @@ -886,7 +886,7 @@ HUF_decompress4X2_usingDTable_internal_body(
>>>>> HUF_DECODE_SYMBOLX2_0(op2, &bitD2);
>>>>> HUF_DECODE_SYMBOLX2_0(op3, &bitD3);
>>>>> HUF_DECODE_SYMBOLX2_0(op4, &bitD4);
>>>>> - endSignal = (U32)LIKELY(
>>>>> + endSignal = (U32)LIKELY((U32)
>>>>> (BIT_reloadDStreamFast(&bitD1) == BIT_DStream_unfinished)
>>>>> & (BIT_reloadDStreamFast(&bitD2) == BIT_DStream_unfinished)
>>>>> & (BIT_reloadDStreamFast(&bitD3) == BIT_DStream_unfinished)
>>>
>>> Isn't that the same as:
>>> ((BIT_reload() & BIT_reload() & BIT_reload()) == BIT_DStream_unfinished)
>>> which will generate much better code.
>>> Especially on cpu without 'seteq' instructions.
>>
>> I don't think so. Feel free to double check my math.
>>
>> BIT_reloadDStreamFast() can return either BIT_DStream_unfinished (0) or
>> BIT_DStream_overflow (3)....
>
> Ah, I'd assumed that BIT_DStream_unfinished was non-zero.
> So you actually want:
> endSignal = !(BIT() | BIT() | BIT());
>
> Just kill the CaMeLs and unnecessary constants.
> Then the code becomes succint, easier to read/check etc.
`BIT_reloadDStreamFast()` has a likely branch which returns `BIT_DStream_unfinished`.
This construction is telling the compiler that it is allowed to re-order each call and collect
the results. I don’t expect that it will translate directly to a set of and instructions, though
I’d have to double check the assembly to be sure.
If you feel the code could be clearer, you’re welcome to submit a PR upstream! However,
since is a hot loop, we generally favor performance over clarity to some extent, so it will
have to be a perf neutral refactoring.
Best,
Nick Terrell
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>