ECAL - Make EcalConstants constexpr arrays compatible with CUDA code #37161

thomreis · 2022-03-07T18:47:26Z

PR description:

The constexpr arrays defined in EcalConstants.h can not be used in CUDA kernels because they are no scalar types. With the HOST_DEVICE_CONSTANT defined in #37159 it is possible to define the arrays in device memory and use the values.

However, using HOST_DEVICE_CONSTANT is not possible if the arrays are members of a class and so they are moved to namespaces and constant pointers to the arrays are used in the classes (#36311 (comment)).

This PR builds on PR #37159

PR validation:

Compiles and passes 11634.512 on a CPU-only machine and on an GPU equipped machine.

cmsbuild · 2022-03-07T18:56:09Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37161/28721

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HeterogeneousCore/CUDAUtilities/interface/HostDeviceConstant.h modified in PR(s): Define HOST_DEVICE_CONSTANT to implement constexpr aggregate host/device constants #37159

cmsbuild · 2022-03-07T18:56:28Z

A new Pull Request was created by @thomreis (Thomas Reis) for master.

It involves the following packages:

DataFormats/EcalDigi (simulation)
HeterogeneousCore/CUDAUtilities (heterogeneous)

@cmsbuild, @makortel, @civanch, @mdhildreth, @fwyzard can you please review it and eventually sign? Thanks.
@makortel, @rovere, @argiro, @rchatter, @thomreis, @simonepigazzini 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

makortel · 2022-03-07T19:52:31Z

DataFormats/EcalDigi/BuildFile.xml

@@ -3,6 +3,7 @@
 <use name="DataFormats/DetId"/>
 <use name="FWCore/MessageLogger"/>
 <use name="FWCore/Utilities"/>
+<use name="HeterogeneousCore/CUDAUtilities"/>


Unfortunately DataFormats package depending on HeterogeneousCore is not allowed (in order to avoid dependence on CUDA in persistent data formats).

The header HostDeviceConstant.h itself depends on CUDA only "weakly" though (it only uses macros that are defined by CUDA-capable compiler and as such it does not require CUDA by itself). Maybe that header should be placed somewhere in DataFormats instead? (assuming EcalConstants.h can not be moved to non-dataformat package)

What about moving it to FWCore/Utilities, like FWCore/Utilities/interface/CMSUnrollLoop.h ?

That would work too.

OK, I've updated #37159 accordingly.

I have updated this one as well.

cmsbuild · 2022-03-08T09:11:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37161/28734

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Utilities/interface/HostDeviceConstant.h modified in PR(s): Define HOST_DEVICE_CONSTANT to implement constexpr aggregate host/device constants #37159

cmsbuild · 2022-03-08T09:12:06Z

Pull request #37161 was updated. @smuzaffar, @civanch, @Dr15Jones, @makortel, @mdhildreth, @cmsbuild can you please check and sign again.

fwyzard · 2022-03-08T10:15:10Z

please test

cmsbuild · 2022-03-08T14:32:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b0ee52/22930/summary.html
COMMIT: c3d3e02
CMSSW: CMSSW_12_3_X_2022-03-07-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37161/22930/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: 7 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3985378
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3985344
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

civanch · 2022-03-08T16:10:46Z

+1

perrotta · 2022-03-09T17:35:52Z

please test

cmsbuild · 2022-03-09T22:19:12Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b0ee52/22986/summary.html
COMMIT: a154f9e
CMSSW: CMSSW_12_3_X_2022-03-09-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37161/22986/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: 2 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3703700
DQMHistoTests: Total failures: 11
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3703666
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 48 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

thomreis · 2022-03-10T08:32:44Z

Hi @cms-sw/simulation-l2 can you please sign this again?

civanch · 2022-03-10T08:59:23Z

+1

cmsbuild · 2022-03-10T08:59:44Z

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)

smuzaffar · 2022-03-10T14:16:42Z

@civanch , do you understand why we now see random differences relval comparisons for Generator e.g in #37161 (comment) ( the the details at https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2022-03-09-1100+b0ee52/48838/validateJR.html ) .

perrotta · 2022-03-10T14:37:04Z

@civanch , do you understand why we now see random differences relval comparisons for Generator e.g in #37161 (comment) ( the the details at https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_12_3_X_2022-03-09-1100+b0ee52/48838/validateJR.html ) .

@smuzaffar the idea is that those random differences should disappear after the merging of #37185, since CMSSW_12_3_X_2022-03-10-1100

Since that IB should be already available for the tests, we can relaunch them to verify if it is actually the case

perrotta · 2022-03-10T14:37:33Z

please test

cmsbuild · 2022-03-10T19:05:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b0ee52/23020/summary.html
COMMIT: a154f9e
CMSSW: CMSSW_12_3_X_2022-03-10-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37161/23020/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3695161
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3695125
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 204 log files, 45 edm output root files, 49 DQM output files
TriggerResults: no differences found

perrotta · 2022-03-10T21:46:06Z

Random differences in the generator weights have disappeared now, after the merging in the IB of #37185

qliphy · 2022-03-13T01:33:45Z

+1

cmsbuild added this to the CMSSW_12_3_X milestone Mar 7, 2022

cmsbuild added code-checks-pending heterogeneous-pending orp-pending pending-signatures simulation-pending tests-pending labels Mar 7, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 7, 2022

makortel reviewed Mar 7, 2022

View reviewed changes

thomreis force-pushed the ecal-constants-for-gpu-code branch from 9791e74 to c3d3e02 Compare March 8, 2022 09:03

cmsbuild added code-checks-pending core-pending and removed code-checks-approved heterogeneous-pending labels Mar 8, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 8, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 8, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 8, 2022

cmsbuild added simulation-approved and removed simulation-pending labels Mar 8, 2022

cmsbuild added tests-started and removed tests-pending labels Mar 9, 2022

cmsbuild added tests-approved and removed tests-started labels Mar 9, 2022

cmsbuild added fully-signed simulation-approved and removed simulation-pending pending-signatures labels Mar 10, 2022

cmsbuild added tests-started and removed tests-approved labels Mar 10, 2022

perrotta mentioned this pull request Mar 10, 2022

[Do not merge] testing bot cms-sw/cmsdist#7676

Closed

cmsbuild added tests-approved and removed tests-started labels Mar 10, 2022

smuzaffar modified the milestones: CMSSW_12_3_X, CMSSW_12_4_X Mar 11, 2022

cmsbuild added orp-approved and removed orp-pending labels Mar 13, 2022

cmsbuild merged commit baa7e7b into cms-sw:master Mar 13, 2022

thomreis deleted the ecal-constants-for-gpu-code branch March 13, 2022 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECAL - Make EcalConstants constexpr arrays compatible with CUDA code #37161

ECAL - Make EcalConstants constexpr arrays compatible with CUDA code #37161

thomreis commented Mar 7, 2022

cmsbuild commented Mar 7, 2022

cmsbuild commented Mar 7, 2022

makortel Mar 7, 2022

fwyzard Mar 7, 2022

makortel Mar 7, 2022

fwyzard Mar 7, 2022

thomreis Mar 8, 2022

cmsbuild commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

fwyzard commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

civanch commented Mar 8, 2022

perrotta commented Mar 9, 2022

cmsbuild commented Mar 9, 2022

thomreis commented Mar 10, 2022

civanch commented Mar 10, 2022

cmsbuild commented Mar 10, 2022

smuzaffar commented Mar 10, 2022

perrotta commented Mar 10, 2022

perrotta commented Mar 10, 2022

cmsbuild commented Mar 10, 2022

perrotta commented Mar 10, 2022

qliphy commented Mar 13, 2022

ECAL - Make EcalConstants constexpr arrays compatible with CUDA code #37161

ECAL - Make EcalConstants constexpr arrays compatible with CUDA code #37161

Conversation

thomreis commented Mar 7, 2022

PR description:

PR validation:

cmsbuild commented Mar 7, 2022

cmsbuild commented Mar 7, 2022

makortel Mar 7, 2022

Choose a reason for hiding this comment

fwyzard Mar 7, 2022

Choose a reason for hiding this comment

makortel Mar 7, 2022

Choose a reason for hiding this comment

fwyzard Mar 7, 2022

Choose a reason for hiding this comment

thomreis Mar 8, 2022

Choose a reason for hiding this comment

cmsbuild commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

fwyzard commented Mar 8, 2022

cmsbuild commented Mar 8, 2022

Comparison Summary

civanch commented Mar 8, 2022

perrotta commented Mar 9, 2022

cmsbuild commented Mar 9, 2022

Comparison Summary

thomreis commented Mar 10, 2022

civanch commented Mar 10, 2022

cmsbuild commented Mar 10, 2022

smuzaffar commented Mar 10, 2022

perrotta commented Mar 10, 2022

perrotta commented Mar 10, 2022

cmsbuild commented Mar 10, 2022

Comparison Summary

perrotta commented Mar 10, 2022

qliphy commented Mar 13, 2022