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

Drop -mrecip completely. #5107

Merged
merged 1 commit into from Sep 2, 2014
Merged

Drop -mrecip completely. #5107

merged 1 commit into from Sep 2, 2014

Conversation

ktf
Copy link
Contributor

@ktf ktf commented Aug 29, 2014

-mrecip optimizes x/y to x * 1/y which can be convenient on some CPUs at the cost of slightly different results. This was introduced a few weeks ago by @VinInn due to alleged performance improvements that turned out to be not as large when running larger scale tests. Given it is not supported on clang we (@VinInn and myself) agreed to drop it for the moment. Notice also that probably -freciprocal-math provides the same level of optimization and it is also available on clang.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ktf (Giulio Eulisse) for CMSSW_7_2_X.

Drop -mrecip completely.

It involves the following packages:

RecoEgamma/EgammaPhotonAlgos
RecoVertex/PrimaryVertexProducer
TrackingTools/GsfTools
TrackingTools/GsfTracking
TrackingTools/KalmanUpdators
TrackingTools/TrackFitters
TrackingTools/TrajectoryState

@nclopezo, @cmsbuild, @Degano, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@battibass, @abbiendi, @GiacomoSguazzoni, @rovere, @VinInn, @bellan, @gpetruc, @cerati, @lgray, @trocino, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.

@slava77
Copy link
Contributor

slava77 commented Aug 29, 2014

Now I'm lost.
@VinInn Vincenzo, wouldn't we loose in CPU-s/evt here?

@ktf
Copy link
Contributor Author

ktf commented Aug 29, 2014

See my comment in #5103. Vincenzo agreed in a private discussion.

@StoyanStoynev
Copy link
Contributor

@ktf Giulio please put some relevant description in the PR.

@cmsbuild
Copy link
Contributor

@ktf
Copy link
Contributor Author

ktf commented Aug 29, 2014

Done.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Sep 1, 2014

+1

for #5107 e1e62cc
compared to CMSSW_7_2_X_2014-08-28-1400 as a baseline (test area sign419)
All changes are after all a removal of -mrecip, which is supposedly for the better, more precise.
The effect is visible in physics outputs at numerical precision level and doesn't seem to have any problematic change.

No visible effect on CPU.

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 1, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ktf added a commit that referenced this pull request Sep 2, 2014
@ktf ktf merged commit 4b16e58 into cms-sw:CMSSW_7_2_X Sep 2, 2014
@ktf ktf deleted the drop-mrecip branch September 2, 2014 05:16
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

4 participants