-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use ofast-flag rather than fast-math for mkFit #41374
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41374/35230
|
A new Pull Request was created by @mmasciov (Mario Masciovecchio) for master. It involves the following packages:
@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b29b55/32045/summary.html Comparison SummarySummary:
|
enable profiling |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b29b55/32086/summary.html Comparison SummarySummary:
|
@clacaputo, thanks for enabling and triggering tests with profiling. |
I had to resubmit some of the profiling jobs which might have caused the missing |
I noticed the empty |
@gartung, @clacaputo, is there any news about this (as it's the only thing holding this PR to my understanding)? |
You can try re-running the PR tests. It might work this time. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b29b55/32234/summary.html Comparison SummarySummary:
|
@clacaputo I haven't figured out why the Igprof cpu profiles are empty. For now I would use the Resources Pier Charts. They are the json files linked in the profiling summary for each workflow. |
Hi @gartung , thanks for checking. Unfortunately, after clicking on the json, the Circles page is empty |
and |
I've tested the PR on The TimeDiff
There is slight increase in reco timing that could be a fluctuation. |
@clacaputo, indeed we do not expect any change beyond fluctuations, as also confirmed in our validation (see, e.g., http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/testFastMath/MTV_ttbarPU_cgpu-1_fastmath_vs_ofast/plots_timing/summaryReal.pdf). |
+reconstruction |
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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
As per title, this PR is meant to switch to
ofast-flag
rather thanfast-math
for mkFit building, as this was proven to allow for improved stability across different architectures.An analogous change is applied to the standalone mkFit
Makefile.config
.Additional notes on the mkFit
Makefile.config
:ofast-flag
in CMSSW consists of-Ofast -fno-reciprocal-math -mrecip=none
(see cms-sw/cmsdist#8280).This is reflected in the mkFit
Makefile.config
, with intentional redundancy:-Ofast
turns on-ffast-math
, which in turn activates-fno-math-errno
(see reference); the redundant flags are not removed intentionally, so that in the case one is willing to remove the effects ofofast-flag
in the standalone mkFit application, it will be sufficient to comment out the line added inMakefile.config
with this PR, without changing vectorization due to other pre-existing flags.PR validation:
This PR is not meant to affect physics performance, except for compilation-driven effects. Technical performance is not affected either.
This is validated in: http://uaf-10.t2.ucsd.edu/~mmasciov/MIC/testFastMath/MTV_ttbarPU_cgpu-1_fastmath_vs_ofast/ (red/black vs. blue)
FYI, @mmusich, @slava77