[LTO - RecoTracker] Solve -Wstrict-overflow compiler warnings #38683
Conversation
|
assign reconstruction |
|
@cmsbuild , please test for CMSSW_12_5_LTO_X |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38683/30982
|
|
A new Pull Request was created by @aandvalenzuela (Andrea Valenzuela) for master. It involves the following packages:
@jpata, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -13,7 +13,7 @@ inline void FastLinearCMNSubtractor::subtract_(uint32_t detId, uint16_t firstAPV | |||
| tmp.reserve(128); | |||
| typename std::vector<T>::iterator strip(digis.begin()), end(digis.end()), endAPV, high, low; | |||
|
|
|||
| while (strip < end) { | |||
| while (strip - end < 0) { | |||
There was a problem hiding this comment.
looking at https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator
why would a semantically equivalent expression trigger a warning?
There was a problem hiding this comment.
Hi,
I am also not 100% sure, but we are observing this warning because of the same reason multiple times in different modules (for example, #38649). Also, it is not a transient warning since it appears consistently in our IBs. I was following the discussion I mentioned before and their suggested fixes (fmtlib/fmt@8e2e4d4). Feel free to propose any other solution to get rid of this type of warnings.
I am reproducing the warnings using CMSSW_12_5_LTO_X IBs.
Many thanks,
Andrea.
There was a problem hiding this comment.
Hi,
After some further investigation, I think in this case the compiler is warning about a possible pointer wraparound since, for example, when checking high < endAPV, it is in reality checking for:
endAPV = strip + 128;
high = strip + 64;
My fear about high - endAPV < 0 is that the warning goes away, but probably also the optimization and that is why the warning is not shown anymore.
There was a problem hiding this comment.
this is what I understood reading about similar reports from 2014
There was a problem hiding this comment.
I think (based on coming to similar conclusion, based on e.g. https://www.spinics.net/lists/gcchelp/msg52198.html that shows an example where it is easy to see this kind of change prevents the optimization) we should probably think more what to do in these cases.
The warning basically says that GCC assumed no pointer or signed integer wraparound and optimized code based on that. If the code has no way under/overflow, in the leading order the logic would not need a fix (the documentation also mentions false positives). Some tuning of the code might still be useful to make GCC's optimization path more clear.
But I'm not sure what would the best approach for cases where the code is deemed correct. Is there anything better than locally disabling the warning (which will then clutter the code)?
There was a problem hiding this comment.
Hi @makortel,
Many thanks for your answer. I was also thinking on the option of disabling the warnings only on the specific code locations where it appears. I have been doing some trials with #pragma GCC diagnostic ignored and #pragma push and pop, but those pragma declarations seem to be ignored with the LTO build. I have been searching and it seems this happens since the compiler is invoked back at link stage in these cases.
If we finally decide to disable the warning, is there any other approach to target these warnings locally?
Thank you!
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-73d387/26135/summary.html Comparison SummarySummary:
|
|
From what we discussed in the Core Software meeting, I understood we can go ahead with this PR and then we will evaluate if these changes suppose a significant loose in performance or not. |
|
I'm personally not in favor of changing code that is clearly well formed and correct to a more complex and less intuitive form. |
|
Hi @aandvalenzuela I can see some minimal reco differences, but I suppose they are related to having the comparison baseline IB |
|
Hi @clacaputo, The LTO IB has some compiler optimizations of this kind (Linked Time Optimization). It allows the compiler to optimize entire libraries as if they were single compilation units. I think it is based on the work done by Niccolò Forzano. I attach the link to two talks that he gave explaining how the optimization (both LTO and PGO) works and the improvements we can gain from it: I hope it helps :) Many thanks, |
|
+reconstruction
side note: we should go in the direction of what was stated in #38683 (comment) |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
reconstruction-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
I tend to agree with the @VinInn statement in #38683 (comment) |
|
assign core |
|
New categories assigned: core @Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks |
|
I feel uneasy (as well) with changes preventing optimizations just to silence informative-only compiler warnings. I understand (and agree) with the desire to get the LTO build free from compilation warnings, but we have other code for which it appears to be difficult to come up with this kind of transformation. I'd like (us) to spend a bit more time to understand the codes in question if a different solution could be found to silence the warnings. |
|
hold |
|
Pull request has been put on hold by @perrotta |
|
|
Hello,
We have seen some compiler warnings of the type
-Wstrict-overflowin LTO_X IBs (CMSSW_12_5_LTO_X_2022-07-07-1100 and CMSSW_12_5_LTO_X_2022-07-06-1100, for example) inRecoLocalTracker/SiStripZeroSuppression,RecoTracker/FinalTrackSelectorsandRecoTracker/MkFitCMSpackages. See sample stack traces, respectively:RecoLocalTracker/SiStripZeroSuppression:RecoTracker/FinalTrackSelectors:RecoTracker/MkFitCMS:Regarding the type of error and following some online discussions of this type of warning fmtlib/fmt/issues/2757, I have tried to workaround it as shown in this PR for
RecoLocalTracker/SiStripZeroSuppressionandRecoTracker/FinalTrackSelectors. ForRecoTracker/MkFitCMSI just corrected the narrowing cast. Feel free to propose other solutions that could be better regarding the analysis itself.Many thanks,
Andrea.