Skip to content

Updated SoA View accessors from raw pointers to span #48377

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

leobeltra
Copy link

@leobeltra leobeltra commented Jun 20, 2025

PR description:

This PR updates the column accessors in the View from raw pointers to std::span<T>, providing automatic range checking. Adjustments have also been made in CMSSW to support this new approach. This PR is dependent on #48216, so it should be merged after that.

PR validation:

The unit tests in CMSSW still pass.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 20, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48377/45264

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @leobeltra for master.

It involves the following packages:

  • CUDADataFormats/TrackingRecHit (heterogeneous, reconstruction)
  • CondFormats/EcalObjects (db, alca)
  • CondFormats/HGCalObjects (db, upgrade, alca)
  • CondFormats/HcalObjects (db, alca)
  • DQM/SiPixelHeterogeneous (dqm)
  • DataFormats/HGCalDigi (upgrade, simulation)
  • DataFormats/HGCalRecHit (upgrade, reconstruction)
  • DataFormats/HGCalReco (upgrade, reconstruction)
  • DataFormats/ParticleFlowReco (reconstruction)
  • DataFormats/Portable (heterogeneous)
  • DataFormats/PortableTestObjects (heterogeneous)
  • DataFormats/SiPixelDigi (simulation)
  • DataFormats/SiPixelDigiSoA (heterogeneous, reconstruction)
  • DataFormats/SoATemplate (heterogeneous)
  • DataFormats/TrackingRecHitSoA (heterogeneous, reconstruction)
  • EventFilter/EcalRawToDigi (reconstruction)
  • HeterogeneousCore/AlpakaTest (heterogeneous)
  • HeterogeneousCore/CUDATest (heterogeneous)
  • RecoLocalCalo/EcalRecProducers (reconstruction)
  • RecoLocalCalo/HGCalRecAlgos (upgrade, reconstruction)
  • RecoLocalCalo/HGCalRecProducers (upgrade, reconstruction)
  • RecoLocalCalo/HcalRecProducers (reconstruction)
  • RecoLocalTracker/SiPixelClusterizer (reconstruction)
  • RecoLocalTracker/SiPixelRecHits (reconstruction)
  • RecoParticleFlow/PFClusterProducer (reconstruction)
  • RecoParticleFlow/PFRecHitProducer (reconstruction)
  • RecoTauTag/HLTProducers (hlt)
  • RecoTracker/LSTCore (reconstruction)
  • RecoTracker/PixelSeeding (reconstruction)
  • RecoTracker/PixelTrackFitting (reconstruction)
  • RecoTracker/TkSeedGenerator (reconstruction)
  • RecoVertex/PixelVertexFinding (reconstruction)

@Martin-Grunewald, @Moanwar, @antoniovagnerini, @atpathak, @civanch, @cmsbuild, @ctarricone, @francescobrivio, @fwyzard, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @mmusich, @perrotta, @rseidita, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @JanChyczynski, @Martin-Grunewald, @PonIlya, @ReyerBand, @VinInn, @VourMa, @abdoulline, @apsallid, @argiro, @azotz, @bsunanda, @cseez, @denizsun, @dgulhan, @dkotlins, @edjtscott, @fabiocos, @felicepantaleo, @ferencek, @fioriNTU, @gpetruc, @hatakeyamak, @idebruyn, @jandrea, @lecriste, @lgray, @makortel, @mariadalfonso, @martinamalberti, @mbluj, @missirol, @mmarionncern, @mmusich, @mroguljic, @mtosi, @pfs, @rchatter, @rovere, @rsreds, @salimcerci, @sameasy, @seemasharmafnal, @sethzenz, @thomreis, @threus, @tocheng, @tsusa, @tvami, @vandreev11, @wang0jin, @youyingli, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented Jun 20, 2025

type ngt

@mmusich
Copy link
Contributor

mmusich commented Jun 20, 2025

enable gpu

@mmusich
Copy link
Contributor

mmusich commented Jun 20, 2025

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented Jun 20, 2025

allow @leobeltra test rights

@leobeltra leobeltra changed the title Updated SoA accessors view from raw pointers to span Updated SoA View accessors from raw pointers to span Jun 20, 2025
<< " id @ " << view.metadata().addressOf_id() << " = " << Column(view.id(), view.metadata().size()) << ",\n"
<< " r @ " << view.metadata().addressOf_r() << " = " << view.r() << '\n'
<< " flags @ " << view.metadata().addressOf_flags() << " = " << Column(view.flags(), view.metadata().size())
<< " x @ " << view.metadata().addressOf_x() << " = " << Column(view.x().data(), view.metadata().size())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a constructor for Column that takes a span as a single parameter, and change the code here to use it ?

@@ -76,8 +76,8 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE::ecal::multifit {
// for accessing input arrays
int const inputTx = ch >= nchannelsEB ? idx.global - nchannelsEB * nsamples : idx.global;
// eb is first and then ee
auto const* digis_in = ch >= nchannelsEB ? digisDevEE.data()->data() : digisDevEB.data()->data();
auto const gainId = ecalMGPA::gainId(digis_in[inputTx]);
auto const& digis_in = ch >= nchannelsEB ? digisDevEE.data() : digisDevEB.data();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the span is constructed on the fly, so there is no advantage to using a reference.
can you remove the & for consistency with the other changes you made?

@@ -345,7 +344,7 @@ namespace ALPAKA_ACCELERATOR_NAMESPACE::ecal::multifit {
constexpr bool simplifiedNoiseModelForGainSwitch = true; //---- default is true

// pulse matrix
auto const* pulse_shapes = reinterpret_cast<const EcalPulseShape*>(conditionsDev.pulseShapes()->data());
auto const pulse_shapes = reinterpret_cast<const EcalPulseShape*>(conditionsDev.pulseShapes().data());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove the * ?

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48377/45271

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48377/45272

@cmsbuild
Copy link
Contributor

@leobeltra
Copy link
Author

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals RelVals-ROCM rocmUnitTests
Size: This PR adds an extra 40KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4f8d28/46860/summary.html
COMMIT: 6bbea40
CMSSW: CMSSW_15_1_X_2025-06-21-1100/el8_amd64_gcc12
Additional Tests: CUDA,ROCM
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/48377/46860/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

  • 145.5A fatal system signal has occurred: segmentation violation

RelVals-ROCM

  • 12834.40312834.403_TTbar_14TeV+2024_Patatrack_PixelOnlyAlpaka_Validation/step3_TTbar_14TeV+2024_Patatrack_PixelOnlyAlpaka_Validation.log

ROCm Unit Tests

I found 1 errors in the following unit tests:

---> test testRocmSoALayoutAndView_t had ERRORS

CUDA Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 62 differences found in the comparisons
  • DQMHistoTests: Total files compared: 7
  • DQMHistoTests: Total histograms compared: 53212
  • DQMHistoTests: Total failures: 3759
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 49453
  • DQMHistoTests: Total skipped: 0
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 6 files compared)
  • Checked 24 log files, 30 edm output root files, 7 DQM output files
  • TriggerResults: no differences found

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.

4 participants