Skip to content
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

RecoTracker/MkfitCore: Add fast-math and function attribute need to prevent segfault #38147

Merged

Conversation

gartung
Copy link
Member

@gartung gartung commented May 31, 2022

Add fast-math flag and attribute needed to prevent segfault when fast-math is used

This PR adds back the options which enable the vectorized function from libmvec on EL8. This causes a differences in RECO DQM.

Followup PR to #37868

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38147/30290

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @gartung (Patrick Gartung) for master.

It involves the following packages:

  • RecoTracker/MkFitCore (reconstruction)

@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @ebrondol, @gpetruc, @mmusich, @mtosi, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@gartung
Copy link
Member Author

gartung commented May 31, 2022

enable profiling

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38147/30296

  • This PR adds an extra 28KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38147/30297

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

Pull request #38147 was updated. @jpata, @cmsbuild, @clacaputo, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-77cf4a/25125/summary.html
COMMIT: 19b46ed
CMSSW: CMSSW_12_5_X_2022-05-31-1100/el8_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38147/25125/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10965 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3648315
  • DQMHistoTests: Total failures: 14532
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3633760
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 6, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-77cf4a/25291/summary.html
COMMIT: d3576b6
CMSSW: CMSSW_12_5_X_2022-06-05-0000/el8_amd64_gcc10
Additional Tests: PROFILING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/38147/25291/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 10969 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3651603
  • DQMHistoTests: Total failures: 14536
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3637044
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

clacaputo commented Jun 8, 2022

type tracking

@clacaputo
Copy link
Contributor

assign tracking-pog

The reco differences introduced by this PR seems minimal to me, but I'd like to have a crosscheck by tracking-pog

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2022

New categories assigned: tracking-pog

@mmusich,@vmariani,@mtosi you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Jun 10, 2022

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-77cf4a/25291/summary.html


Additional Tests: PROFILING

Looking at the profile, the mkFit related calls got down from 15.26 ticks to 13.81. So, this is about 10% reduction.

@slava77
Copy link
Contributor

slava77 commented Jun 11, 2022

@clacaputo
Copy link
Contributor

Hi @gartung @slava77 , do we want to backport it for 12_4_X?

@slava77
Copy link
Contributor

slava77 commented Jun 13, 2022

Hi @gartung @slava77 , do we want to backport it for 12_4_X?

this PR is not purely technical and differences in physics are visually larger than what you'd usually see with small shuffle at the numerical precision level.

e.g. the number of tracks in detached triplet and pixelLess iterations went down a bit, as can be seen in DQM bin-to-bin for wf 136.874 https://tinyurl.com/2d3gpg9c
image
this is consistent with an observation from a ttbar MC sample that the fake rate in these iterations goes down @leonardogiannini please share a link

So, it may make more sense to have this formally validated first and then backported.

@leonardogiannini
Copy link
Contributor

this is the validation on 10 muon sample (pT 0.2 to 1000)
http://uaf-10.t2.ucsd.edu/~legianni/Check-PR38147-fastmath-10mu-112/

ttbar with PU sample
http://uaf-10.t2.ucsd.edu/~legianni/Check-PR38147-fastmath-112/

red only adds the PR 38147 to the code wrt to blue.

@clacaputo
Copy link
Contributor

@mmusich
Copy link
Contributor

mmusich commented Jun 14, 2022

@cmsbuild
Copy link
Contributor

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 (and backports should be raised in the release meeting by the corresponding L2)

@perrotta
Copy link
Contributor

+1

  • As discussed at tody's ORP meeting, it needs to be better validated before thinking about possible backports

@cmsbuild cmsbuild merged commit e0e63fa into cms-sw:master Jun 14, 2022
@gartung gartung deleted the gartung-mkFit-sse3-vectorize-fastmath branch August 22, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants