search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.08k stars 4.29k forks source link

LLVM 14: bitwise-instead-of-logical warnings #39105

Open smuzaffar opened 2 years ago

smuzaffar commented 2 years ago

LLVM 14 tests shows a lot (over 30K) bitwise-instead-of-logical warning types . Some of these are in [a]. @VinInn mentioned here that some time using bitwise if faster then logical operation.

What should we do with these warnings? should we replace all bitwise operators to logical or disabled the warning for selected cases and fix the rest by using logical operators?

[a]

grep ': warning:' build.log | grep CMSSW_12_5_CLANG_X_2022-08-17-2300/src/ | sort | uniq  | grep -v 'bitwise-instead-of-logical' |wc -l
215
   1050 DataFormats/GeometrySurface/interface/SimpleDiskBounds.h:21:13: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1050 DataFormats/GeometrySurface/interface/SimpleDiskBounds.h:22:13: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1578 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:30:13: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:31:12: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:35:64: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:36:62: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:37:58: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1581 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:39:14: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1584 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:36:63: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
   1584 DataFormats/TrackerRecHit2D/interface/trackerHitRTTI.h:39:13: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical]
  12924 DataFormats/GeometrySurface/interface/GloballyPositioned.h:157:9: warning: use of bitwise '&' with boolean operands [-Wbitwise-instead-of-logical]
smuzaffar commented 2 years ago

assign core

cmsbuild commented 2 years ago

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild commented 2 years ago

A new Issue was created by @smuzaffar Malik Shahzad Muzaffar.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel commented 2 years ago

A quick search suggests either the use of logical operations, or explicit cast to (unsigned) int for either operand to silence the warning (for the cases where the behavior of bitwise operation is desired).

The performance impact is really a question of tradeoff between the cost of a branch (logical operation) vs the cost of always executing the right-hand side (bitwise operation).

By quick look on those three files, the use of bitwise operations could be legitimate (at least the boolean expressions do not contain any side effects).

Dr15Jones commented 2 years ago

@makortel did you check in godbolt what the optimizer does? I'd bet it does the same operations for bool used for & or &&.

Dr15Jones commented 2 years ago

did you check in godbolt what the optimizer does? I'd bet it does the same operations for bool used for & or &&.

Indeed, they are the same

https://godbolt.org/z/qc3hcqPnT

makortel commented 2 years ago

Seems that things have improved in ~10 years :) I remember seeing (in kd-tree implementation) a noticeable impact on performance back then. Going back with godbolt I see gcc 4.6 still emits a branch with &&, whereas gcc 4.7 results in equal code. (for clang I see the same between versions 3.6 and 3.7)

On the other hand, a bit more complicated with comparison expressions still shows a branch with && https://godbolt.org/z/cqdvsTr7P

VinInn commented 2 years ago

again: The use of bitwise operators IS intentional! logical operators force the compiler to evaluate them in sequence as the following maybe ill formed if previous fails (for instance p&&(*p).ok()) and introduce branches while bitwise operators can be evaluated in any order (even in parallel) and do not require branches. I'm very sorry for llvm but coders cannot loose their mind and time to check for each single version and type of compiler what is emitted. I know what I'm doing, please compiler just respect my wish.