[LLVM10] Fixing compilation warnings for Reco and SIM #29568

smuzaffar · 2020-04-28T08:43:46Z

PR description:

LLVM 10 integration in CLANG IBs shows new compilation warnings. This PR addresses few of those.

PR validation:

Local compilation for CLANG and normal IBs show no warnings.

cmsbuild · 2020-04-28T08:44:12Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-04-28T08:50:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29568/14874

This PR adds an extra 52KB to repository

cmsbuild · 2020-04-28T08:51:06Z

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

DataFormats/Math
RecoLocalTracker/SiStripZeroSuppression
RecoMuon/MuonIdentification
RecoTracker/TkTrackingRegions
RecoTracker/TrackProducer

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@echabert, @felicepantaleo, @pieterdavid, @robervalwalsh, @echapon, @alesaggio, @abbiendi, @mmusich, @cericeci, @makortel, @threus, @jhgoh, @dgulhan, @HuguesBrun, @trocino, @rociovilar, @yduhm, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @mschrode, @ebrondol, @fabiocos, @gbenelli, @Fedespring, @calderona, @gpetruc, @folguera this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

perrotta · 2020-04-28T09:05:10Z

please test

cmsbuild · 2020-04-28T09:05:35Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5881/console Started: 2020/04/28 11:06

smuzaffar · 2020-04-28T09:07:42Z

DataFormats/Math/interface/approx_atan2.h

@@ -121,7 +121,7 @@ constexpr float unsafe_atan2f(float y, float x) {

 template <int DEGREE>
 constexpr float safe_atan2f(float y, float x) {
-  return unsafe_atan2f_impl<DEGREE>(y, (y == 0.f) & (x == 0.f) ? 0.2f : x);
+  return unsafe_atan2f_impl<DEGREE>(y, ((y == 0.f) & (x == 0.f)) ? 0.2f : x);


@VinInn , shouldn't it be && instead of & here?

no. & is faster ( @makortel made some tests long ago )

pieterdavid · 2020-04-28T09:56:29Z

RecoLocalTracker/SiStripZeroSuppression/plugins/SiStripZeroSuppression.cc

@@ -164,7 +164,7 @@ inline edm::DetSet<SiStripRawDigi> SiStripZeroSuppression::formatRawDigis(const
  outRawDigis.reserve(rawDigis.size());
  const std::vector<bool>& apvf = algorithms->getAPVFlags();
  uint32_t strip = 0;
-  for (const auto rawDigi : rawDigis) {
+  for (const auto& rawDigi : rawDigis) {


I am not fully up to date with the latest recommendation, but why is this a warning? SiStripRawDigi (and SiStripDigi, in the two changes below) are small (one or two uint16_t, respectively), so the code was intentionally written like this. Or will the compiler pick the most efficient option in any case when using const auto&?

I think regardless of the size of object, LLVM 10 complains that you are creating a copy of object and suggests to use a &

https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc7_amd64_gcc820/CMSSW_11_1_CLANG_X_2020-04-27-2300/RecoLocalTracker/SiStripZeroSuppression

RecoLocalTracker/SiStripZeroSuppression/plugins/SiStripZeroSuppression.cc:167:19: warning: loop variable 'rawDigi' of type 'const SiStripRawDigi' creates a copy from type 'const SiStripRawDigi' [-Wrange-loop-construct] for (const auto rawDigi : rawDigis) { ^ RecoLocalTracker/SiStripZeroSuppression/plugins/SiStripZeroSuppression.cc:167:8: note: use reference type 'const SiStripRawDigi &' to prevent copying for (const auto rawDigi : rawDigis) { ^~~~~~~~~~~~~~~~~~~~ & 1 warning generated.

hope the compiler will optimize either way

IIRC, we have quite a few cases where a copy is OK for small types.
It would be good to understand the underlying logic why it's triggered here.

Playing around with compiler explorer revealed that it is the non-default destructor of SiStripRawDigi

cmssw/DataFormats/SiStripDigi/interface/SiStripRawDigi.h

Line 20 in b3d42e1

~SiStripRawDigi() {}

makes clang to issue the warning. Using the default destructor either by

~SiStripRawDigi() = default;

or removing the explicit definition removes the warning (while keeping the loop variable taken by value).

According to Clang 10.0 release note [a], looks like clang is not treating simple class like SiStripRawDigi as small/trivially copyable types

[a]
https://releases.llvm.org/10.0.0/tools/clang/docs/ReleaseNotes.html#id3 ). So looks like clang 10.0 is not treating

-Wrange-loop-analysis got several improvements. It no longer warns about a copy being made when the result is bound to an rvalue reference. It no longer warns when an object of a small, trivially copyable type is copied. The warning now offers fix-its. Excluding -Wrange loop-bind-reference it is now part of -Wall. To reduce the number of false positives the diagnostic is disabled in macros and template instantiations.

So in principle going for default destructors where possible would be a slightly better solution.

thanks @makortel .
@pieterdavid @slava77 , let me know if you would like me to revert this change and go for default destructor for SiStripRawDigi class.

it sounds like the default constructor is a better solution.
If the compiler were to take this loop as is, the reference solution is supposedly more costly on this type with uint16_t memory footprint. I'm not sure how effective is the compiler optimization to get rid of the reference completely.

cmsbuild · 2020-04-28T10:24:14Z

+1
Tested at: 2d9f8ab
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-57acd7/5881/summary.html
CMSSW: CMSSW_11_1_X_2020-04-27-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-04-28T10:24:19Z

Comparison job queued.

cmsbuild · 2020-04-28T11:51:41Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-57acd7/5881/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2696435
DQMHistoTests: Total failures: 30
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2696086
DQMHistoTests: Total skipped: 319
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

perrotta · 2020-04-28T13:13:13Z

+1

Technical and rather trivial adjustments
Jenkins tests pass

cmsbuild · 2020-04-28T14:42:42Z

Pull request #29568 was updated. @perrotta, @cmsbuild, @civanch, @mdhildreth, @slava77 can you please check and sign again.

cmsbuild · 2020-04-28T14:47:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-04-28T14:52:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29568/14887

This PR adds an extra 60KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoMuon/MuonIdentification/plugins/MuonIdProducer.cc modified in PR(s): [LLVM10] Fixing compilation warnings for Reco2 #29572

cmsbuild · 2020-04-28T14:52:51Z

Pull request #29568 was updated. @perrotta, @cmsbuild, @civanch, @mdhildreth, @slava77 can you please check and sign again.

smuzaffar · 2020-04-28T14:54:35Z

please test

cmsbuild · 2020-04-28T14:54:59Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5889/console Started: 2020/04/28 17:11

cmsbuild · 2020-04-28T16:31:14Z

+1
Tested at: 58fc07f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-57acd7/5889/summary.html
CMSSW: CMSSW_11_1_X_2020-04-28-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-04-28T16:31:16Z

Comparison job queued.

cmsbuild · 2020-04-28T17:59:38Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-57acd7/5889/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2696435
DQMHistoTests: Total failures: 18
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2696098
DQMHistoTests: Total skipped: 319
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

civanch · 2020-04-29T00:22:04Z

+1

slava77 · 2020-04-29T22:50:09Z

RecoLocalTracker/SiStripZeroSuppression/test/SiStripMergeZeroSuppression.cc

@@ -99,7 +99,7 @@ void SiStripMergeZeroSuppression::produce(edm::Event& event, const edm::EventSet
            LogTrace("SiStripMergeZeroSuppression::produce") << "inserting only the digis for not restored APVs"
                                                             << "\n";
            LogTrace("SiStripMergeZeroSuppression::produce") << "size : " << zsModule->size() << "\n";
-            for (const auto itZsMod : *zsModule) {
+            for (const auto& itZsMod : *zsModule) {


shouldn't this be reverted as well? (same for L118)

smuzaffar · 2020-04-30T09:39:21Z

I am closing this and will merge it with #29572

smuzaffar added 2 commits April 28, 2020 10:35

fix llvm 10 bitwise-conditional-parentheses warning

b9bf1bc

fix llvm10 warning: avoid creating a copy

2d9f8ab

cmsbuild added this to the CMSSW_11_1_X milestone Apr 28, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 28, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 28, 2020

cmsbuild added tests-started and removed tests-pending labels Apr 28, 2020

smuzaffar commented Apr 28, 2020

View reviewed changes

pieterdavid reviewed Apr 28, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Apr 28, 2020

cmsbuild added comparison-available and removed comparison-pending labels Apr 28, 2020

cmsbuild removed pending-signatures reconstruction-pending labels Apr 28, 2020

cmsbuild added the code-checks-approved label Apr 28, 2020

fix llvm warnings for sim: avoid loop variable copy

58fc07f

cmsbuild added code-checks-pending and removed code-checks-approved labels Apr 28, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Apr 28, 2020

smuzaffar changed the title ~~[LLVM10] Fixing compilation warnings for Reco~~ [LLVM10] Fixing compilation warnings for Reco and SIM Apr 28, 2020

cmsbuild added tests-started and removed tests-pending labels Apr 28, 2020

cmsbuild mentioned this pull request Apr 28, 2020

DD4hep: SimTracker/TrackerMaterialAnalysis Migration MaterialAccounting #29577

Merged

cmsbuild added tests-approved and removed tests-started labels Apr 28, 2020

cmsbuild added comparison-available and removed comparison-pending labels Apr 28, 2020

cmsbuild added simulation-approved and removed simulation-pending labels Apr 29, 2020

slava77 reviewed Apr 29, 2020

View reviewed changes

smuzaffar closed this Apr 30, 2020

smuzaffar deleted the llvm10-warnings-reco1 branch April 30, 2020 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLVM10] Fixing compilation warnings for Reco and SIM #29568

[LLVM10] Fixing compilation warnings for Reco and SIM #29568

smuzaffar commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

perrotta commented Apr 28, 2020

cmsbuild commented Apr 28, 2020 •

edited

Loading

smuzaffar Apr 28, 2020

VinInn Apr 28, 2020 •

edited

Loading

pieterdavid Apr 28, 2020

smuzaffar Apr 28, 2020

VinInn Apr 28, 2020

slava77 Apr 28, 2020

makortel Apr 28, 2020

smuzaffar Apr 28, 2020

makortel Apr 28, 2020

smuzaffar Apr 28, 2020 •

edited

Loading

slava77 Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

perrotta commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

smuzaffar commented Apr 28, 2020

cmsbuild commented Apr 28, 2020 •

edited

Loading

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

civanch commented Apr 29, 2020

slava77 Apr 29, 2020

smuzaffar commented Apr 30, 2020

[LLVM10] Fixing compilation warnings for Reco and SIM #29568

[LLVM10] Fixing compilation warnings for Reco and SIM #29568

Conversation

smuzaffar commented Apr 28, 2020

PR description:

PR validation:

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

perrotta commented Apr 28, 2020

cmsbuild commented Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

VinInn Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smuzaffar Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

perrotta commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

smuzaffar commented Apr 28, 2020

cmsbuild commented Apr 28, 2020 • edited Loading

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

cmsbuild commented Apr 28, 2020

civanch commented Apr 29, 2020

Choose a reason for hiding this comment

smuzaffar commented Apr 30, 2020

cmsbuild commented Apr 28, 2020 •

edited

Loading

VinInn Apr 28, 2020 •

edited

Loading

smuzaffar Apr 28, 2020 •

edited

Loading

cmsbuild commented Apr 28, 2020 •

edited

Loading