Messages in this thread | | | From | Nick Terrell <> | Subject | Re: [PATCH] lib: zstd: Add cast to silence clang's -Wbitwise-instead-of-logical | Date | Fri, 5 Nov 2021 21:16:07 +0000 |
| |
> On Oct 26, 2021, at 2:04 PM, David Laight <David.Laight@ACULAB.COM> 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) >
| |