Skip to content

Removal of ofast-flag (AMD/Intel differences)#40649

Closed
silviodonato wants to merge 1 commit intocms-sw:masterfrom
silviodonato:removeOfast
Closed

Removal of ofast-flag (AMD/Intel differences)#40649
silviodonato wants to merge 1 commit intocms-sw:masterfrom
silviodonato:removeOfast

Conversation

@silviodonato
Copy link
Contributor

@silviodonato silviodonato commented Jan 31, 2023 •
edited
Loading

PR description:

About one year ago we discovered differences of few percents between AMD and Intel in few HLT paths when rerunning the HLT [1]. The situation got worse when we added the parking di-electron paths with dZ path, which gave a difference of ~8% [2].
Given that:

  • we didn't manage yet to validate the impact on the offline reconstruction [3]
  • we don't have yet a way to check if an event has been reconstructed by an AMD or Intel CPU [4]
  • the parking di-electron paths with dZ will be used heavily in the 2023 menu (~500 Hz)

I propose to remove Ofast from all CMSSW package (except DataFormat/Math/test) since 13_0_0_pre4.

The impact on reco time is 1.1%-1.2% from @cms-sw/reconstruction-l2 measurement [5].
The impact on the HLT timing is negligible.

Caveat: there is an ongoing test by @gparida following @VinInn suggestion [6] about using -Ofast -fno-reciprocal-math -mno-recip.

Warning to release managers: this PR changes many BuildFiles.xml and therefore the compilation takes a lot of time. This might be a problem for people who will need to rebase their PR including this PR.

[1] https://its.cern.ch/jira/browse/CMSHLT-2257
[2] https://docs.google.com/spreadsheets/d/1XRu1sfIYJUQ0e7Z_yJWdGrxTDA31zr33uWPFY54-APc
[3] https://its.cern.ch/jira/browse/PDMVRELVALS-159
[4] #39982 and #30044
[5] #40089
[6] https://its.cern.ch/jira/browse/CMSHLT-2257?focusedCommentId=4686018&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-4686018

cc: @dpiparo @gparida @cms-sw/hlt-l2 @sanuvarghese

PR validation:

Still ongoing

@cmsbuild cmsbuild added this to the CMSSW_13_0_X milestone Jan 31, 2023
@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 31, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40649/33971

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 31, 2023

A new Pull Request was created by @silviodonato (Silvio Donato) for master.

It involves the following packages:

  • RecoEgamma/EgammaElectronAlgos (reconstruction)
  • RecoEgamma/EgammaPhotonAlgos (reconstruction)
  • RecoPixelVertexing/PixelTriplets (reconstruction)
  • RecoTracker/FinalTrackSelectors (reconstruction)
  • RecoTracker/TkDetLayers (reconstruction)
  • RecoTracker/TkHitPairs (reconstruction)
  • RecoVertex/PrimaryVertexProducer (reconstruction)
  • TrackingTools/GsfTools (reconstruction)
  • TrackingTools/GsfTracking (reconstruction)
  • TrackingTools/KalmanUpdators (reconstruction)
  • TrackingTools/MaterialEffects (reconstruction)
  • TrackingTools/TrackFitters (reconstruction)
  • TrackingTools/TrajectoryState (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@VourMa, @felicepantaleo, @jainshilpi, @abbiendi, @CeliaFernandez, @a-kapoor, @lgray, @mmusich, @cericeci, @andrea21z, @JanFSchulte, @jhgoh, @varuns23, @missirol, @HuguesBrun, @trocino, @Sam-Harper, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @ebrondol, @mtosi, @dgulhan, @Prasant1993, @valsdav, @Fedespring, @sobhatta, @afiqaize, @gpetruc, @wrtabb, @sameasy, @ram1123 this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mandrenguyen
Copy link
Contributor

mandrenguyen commented Jan 31, 2023

Shouldn't we first see what -Ofast -fno-reciprocal-math -mno-recip gives before simply removing -Ofast?

@slava77
Copy link
Contributor

slava77 commented Jan 31, 2023

IIUC, this was tested locally, but perhaps a enable profiling can be done for the record?

@clacaputo
Copy link
Contributor

clacaputo commented Feb 1, 2023

enable profiling

@clacaputo
Copy link
Contributor

clacaputo commented Feb 1, 2023

please test

@VinInn
Copy link
Contributor

VinInn commented Feb 1, 2023

I think we should first get results on reproducibility of -Ofast -fno-reciprocal-math -mno-recip.
I like to remind that in any case we will get regression on ARM and Power (in particular until we do not standardize on AVX2/FMA on x86_64 as other experiments are doing...)

@silviodonato
Copy link
Contributor Author

silviodonato commented Feb 1, 2023

Good news @gparida finished the test using -Ofast -fno-reciprocal-math -mno-recip and it solved the problem!
I'm updating the PR.

@missirol
Copy link
Contributor

missirol commented Feb 1, 2023

I propose to remove Ofast from all CMSSW package (except DataFormat/Math/test) since 13_0_0_pre4.

@silviodonato , maybe you missed 1 file; that flag is also used in TrackingTools/MaterialEffects/BuildFile.xml

cmssw/TrackingTools/MaterialEffects/BuildFile.xml

Line 1 in 7a9e56d

<use name="ofast-flag"/>

@silviodonato silviodonato mentioned this pull request Feb 1, 2023
Merged
@silviodonato
Copy link
Contributor Author

silviodonato commented Feb 1, 2023

Closing this PR in favor of cms-sw/cmsdist#8280

@clacaputo
Copy link
Contributor

clacaputo commented Feb 1, 2023

please abort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

code-checks-approved orp-pending pending-signatures reconstruction-pending tests-pending tracking

Projects

None yet

Milestone

CMSSW_13_0_X

Development

Successfully merging this pull request may close these issues.

7 participants

@silviodonato @cmsbuild @mandrenguyen @slava77 @clacaputo @VinInn @missirol