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

History of VecGeom validation in gcc10 #38789

Closed
makortel opened this issue Jul 19, 2022 · 15 comments
Closed

History of VecGeom validation in gcc10 #38789

makortel opened this issue Jul 19, 2022 · 15 comments

Comments

@makortel
Copy link
Contributor

As requested in the ORP meeting today, this issue is to collect the history of VecGeom validation in gcc10.

@makortel
Copy link
Contributor Author

assign core, simulation, geometry, operations

@cmsbuild
Copy link
Contributor

New categories assigned: geometry,core,operations,simulation

@fabiocos,@mdhildreth,@mdhildreth,@qliphy,@davidlange6,@rappoccio,@ianna,@Dr15Jones,@Dr15Jones,@smuzaffar,@perrotta,@makortel,@makortel,@bsunanda,@civanch,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

type documentation

@makortel
Copy link
Contributor Author

makortel commented Jul 19, 2022

The gcc10 vs gcc9 validation was done in 12_0_0_pre4. The issue about the observed GEM DQM differences is #35030.

VecGeom version in 12_0_0_pre4 was 1.1.16
https://github.com/cms-sw/cmsdist/blob/b81759451981b68eb03f3e425138ba649007ad32/vecgeom.spec#L1
The default optimization level was used at the time
https://github.com/cms-sw/cmsdist/blob/b81759451981b68eb03f3e425138ba649007ad32/vecgeom.spec#L20-L37
I suppose that means -O3 (since in cms-sw/cmsdist#7992 -O2 is requested explicitly).

VecGeom was updated to 1.1.17 in 12_1_0_pre4 with cms-sw/cmsdist#7352.

The production architecture was switched to gcc10 in 12_3_0_pre2.

@civanch
Copy link
Contributor

civanch commented Jul 19, 2022

PR #7992 has been merged. It is planned to perform private validation comparing CMSSW_12_5_X_2022-07-19-2300 (VecGeom -O2) versus CMSSW_12_5_X_2022-07-19-1100 (VecGeom -O3).

Backport to 12_4 #7997 is submitted.

@qliphy
Copy link
Contributor

qliphy commented Jul 20, 2022

The above links should be: cms-sw/cmsdist#7992 and cms-sw/cmsdist#7997

@perrotta
Copy link
Contributor

PR #7992 has been merged. It is planned to perform private validation comparing CMSSW_12_5_X_2022-07-19-2300 (VecGeom -O2) versus CMSSW_12_5_X_2022-07-19-1100 (VecGeom -O3).

See also cms-sw/cmsdist#7992 (comment)

@bsunanda
Copy link
Contributor

I created big xml files using the O2 and O3 options. I ran then 1000 ttbar events using either of these options in CMSSW versions with O2 and O3 option. The conclusion is generation of the big xml file is not affected by the compiler otimization option but the results from the SIM results do. I attach a text file with mean differences of the ratio from 1 in 3 cases:
(1) geometry generated using options O2 or O3 and the SIM step done using O3
(2) geometry generated using options O2 or O3 and the SIM step done using O2
(3) geometry generation and SIM step done using options O2 or O3

Only third case the ratio differs from 1 in all the observables. So there is no need to recreate GT's

@civanch
Copy link
Contributor

civanch commented Jul 23, 2022

In simulation of 1st MinBias event, number of hits in each sub-detector, SimVertex, and SimTrack are different between -O2 and -O3 variants of VecGeom compilation. So, only statistical comparison of results is possible and obviously -O2 should be used for production.

@civanch
Copy link
Contributor

civanch commented Jul 25, 2022

TB2006 results are identical for -O2 and -O3. This means that the problem happens not in the barrel calorimeters.
image

@civanch
Copy link
Contributor

civanch commented Aug 4, 2022

+1

The fix is already integrated into 12_4 and 12_5. VecGeom code may be also modified - see https://sft.its.cern.ch/jira/browse/VECGEOM-600

@perrotta
Copy link
Contributor

perrotta commented Oct 12, 2022

@makortel @civanch if I remember correctly the "non aggressive" optimization was finally chosen for VecGeom after the validation, and this can close this issue: correct?

-O2 is indeed still used for the master as of now:
https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_12_6_X/master/vecgeom.spec#L27

@rappoccio FYI

@civanch
Copy link
Contributor

civanch commented Oct 12, 2022

Several C++ experts were looking into the issue (including Vincenzo I.) and cannot identify why the code has problem. The conclusion was that it is a bug in the compiler.

A practical solution was found out : for gcc10 we use -O2 option for VecGeom, for gcc11 it was decided to go to -O3 back.

The issue may be closed.

@makortel
Copy link
Contributor Author

+core

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

No branches or pull requests

6 participants