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

mkFit: standalone build fixes 2 #122

Merged
merged 2 commits into from
Apr 13, 2023
Merged

mkFit: standalone build fixes 2 #122

merged 2 commits into from
Apr 13, 2023

Conversation

srlantz
Copy link

@srlantz srlantz commented Mar 31, 2023

This is in response to issue #121 from @slava77. I believe it takes care of the Makefile problem where putting 2 targets on the same line might result in the rule for those targets being run simultaneously by 2 threads (as @slava77 hypothesized in Skype). The issue seems to occur twice in the Makefile; this PR fixes both. It also addresses the incomplete cleanup issue, where a couple of .cc source files that are created by rootcling during make get left behind by make distclean. I tested the documented procedure inREADME_buildFromCMSSW.txt, and the new Makefile works both on a fresh clone and a directory tree cleaned by make distclean. I leave it to @osschar to check on the goodness of my solution.

If we decide to make this PR into a further PR to cms-sw, then I propose that we also consider changing the default compilation for standalone builds to -Ofast -fno-reciprocal-math -mrecip=none, since this combination has been approved by CMSSW as not harming reproducibility between AMD and Intel platforms. See cms-sw/cmsdist#8280. This combination of options improves the performance of mkFit by quite a lot: my little test with InitialStep runs almost 20% faster when the above options are provided through CXXUSERFLAGS. In general I think our default standalone build should match the default options for the full CMSSW build... but I can't tell if these flags are now the default for CMSSW builds. Can someone please comment on that?

@slava77
Copy link

slava77 commented Mar 31, 2023

If we decide to make this PR into a further PR to cms-sw, then I propose that we also consider changing the default compilation for standalone builds to -Ofast -fno-reciprocal-math -mrecip=none, since this combination has been approved by CMSSW as not harming reproducibility between AMD and Intel platforms. See cms-sw/cmsdist#8280.

note that this cmsdist PR changes only ofast-flag , while in RecoTracker/MkFitCore/BuildFile.xml we use <flags CXXFLAGS="-ffast-math"/> instead of ofast-flag.
Perhaps we should get some consistency.
Or is this somewhat different topics?

@srlantz
Copy link
Author

srlantz commented Mar 31, 2023

note that this cmsdist PR changes only ofast-flag , while in RecoTracker/MkFitCore/BuildFile.xml we use <flags CXXFLAGS="-ffast-math"/> instead of ofast-flag.
Perhaps we should get some consistency.
Or is this somewhat different topics?

Yes, the question of compiler flags is something I should open as a separate issue. In fact, I started to open a trackreco issue about it, but I don't even know what to call it... perhaps "Make standalone compiler options match what CMSSW is doing for -ffast-math or similar" - ? Anyway, I am bringing it up here simply because I don't like to rain teeny-tiny fixes for the mkFit standalone build into cms-sw.

@osschar
Copy link
Collaborator

osschar commented Apr 4, 2023

When all this first came up I did this on my overlap branch:
cms-sw@209ebcd

I think change (bugfix) on line 64 is crucial ... I forgot to change it after renaming DictsDict to WriteMemFileDict (as there are now two dictionary files).

@srlantz
Copy link
Author

srlantz commented Apr 4, 2023

Thanks @osschar, the bugfixes in the commit you mention were already included my PR #119 to this repo, which became cms-sw#40985, which was merged on March 9. Our trackreco fork is sufficiently up to date with respect to CMSSW, and we now have those fixes.

This PR is different, it addresses a subtle make issue in MkFitCMS/standalone/Makefile that was raised by @slava77 in #121. So my request here is just that you make sure my changes to that Makefile are not somehow messing up your intent. For example, I'm pretty sure the Makefile is generating a couple of temporary source files that need to be cleaned up in the cleaning rules.

@osschar
Copy link
Collaborator

osschar commented Apr 4, 2023

Oh, I see you've made this change already ... sorry.
It all looks good, it makes sense to split this into two rules.

@slava77 slava77 merged commit 77b679d into master Apr 13, 2023
@srlantz srlantz deleted the standalone-build-fixes-2 branch October 30, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants