LST: Merge T5s After Building

[GNiendorf]

This PR moves the T5-T5 merging kernel from the end of LST to immediately after T5 building, so that downstream duplicate-cleaning and object-building kernels (e.g. pT5 building) can take advantage of the longer tracklet length. The merging kernel is optimized so the timing change is negligible. The three T5 duplicate-cleaning kernels (RemoveDupQuintupletsAfterBuild, RemoveDupQuintupletsBeforeTC, and CrossCleanT5) are also revisited to make use of the new variable track length: longer tracklets are preferred over shorter ones, and pairs of longer tracklets get tighter cuts before being flagged as duplicates. Net effect on PU200 is a significant improvement in displaced-track efficiency with a slight decrease in fake rate and negligible impact on duplicate rate or timing.

See here for more details: Merge_T5.pdf

c.c. @slava77

[cmsbuild]

cms-bot internal usage

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51021/49459

• There are other open Pull requests which might conflict with changes you have proposed:
  • File RecoTracker/LSTCore/interface/Common.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679
  • File RecoTracker/LSTCore/src/ModuleMethods.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679
  • File RecoTracker/LSTCore/src/alpaka/Quintuplet.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679

[cmsbuild]

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

It involves the following packages:

• RecoTracker/LSTCore (reconstruction)

@Moanwar, @cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please review it and eventually sign? Thanks.
@GiacomoSguazzoni, @VinInn, @VourMa, @dgulhan, @elusian, @felicepantaleo, @gpetruc, @mmasciov, @mmusich, @mtosi, @rovere this is something you requested to watch as well.
@ftenchini, @mandrenguyen, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

[Moanwar]

test parameters:
enable = hlt_p2_integration, hlt_p2_timing
workflows = ph2_hlt

[Moanwar]

@cmsbuild, please test

[cmsbuild]

-1

Failed Tests: Build
Size: This PR adds an extra 100KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5dcb09/53445/summary.html
COMMIT: 1d072cd
CMSSW: CMSSW_17_0_X_2026-05-22-1100/el8_amd64_gcc13
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/51021/53445/install.sh to create a dev area with all the needed externals and cmssw changes.

Failed Build

I found compilation warning when building: See details on the summary page.

[GNiendorf]

@Moanwar Compilation warning should be fixed.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-51021/49462

• There are other open Pull requests which might conflict with changes you have proposed:
  • File RecoTracker/LSTCore/interface/Common.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679
  • File RecoTracker/LSTCore/src/ModuleMethods.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679
  • File RecoTracker/LSTCore/src/alpaka/Quintuplet.h modified in PR(s): LST: add LSTGeometry package and associated ESProducer #50679

[cmsbuild]

Pull request #51021 was updated. @Moanwar, @cmsbuild, @jfernan2, @mandrenguyen, @srimanob can you please check and sign again.

[Moanwar]

@cmsbuild please test

[Moanwar]

@Moanwar Compilation warning should be fixed.

Thanks!

[cmsbuild]

+1

Size: This PR adds an extra 100KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5dcb09/53448/summary.html
COMMIT: de7d518
CMSSW: CMSSW_17_0_X_2026-05-22-1100/el8_amd64_gcc13
Additional Tests: HLT_P2_INTEGRATION,HLT_P2_TIMING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/51021/53448/install.sh to create a dev area with all the needed externals and cmssw changes.

HLT P2 Timing: chart

Comparison Summary

Summary:

• You potentially removed 4 lines from the logs
• ROOTFileChecks: Some differences in event products or their sizes found
• Reco comparison results: 10 differences found in the comparisons
• DQMHistoTests: Total files compared: 68
• DQMHistoTests: Total histograms compared: 4830146
• DQMHistoTests: Total failures: 31641
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 4798485
• DQMHistoTests: Total skipped: 20
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 67 files compared)
• Checked 284 log files, 246 edm output root files, 68 DQM output files
• TriggerResults: found differences in 4 / 66 workflows

[GNiendorf]

Failed Build

I found compilation warning when building: See details on the summary page.

I think this previous issue is a false positive from -Wstringop-overflow/-Warray-bounds by the way. The reported sizes don't match the actual StdArray types. I used a workaround to silence it, pulling the row refs out of the loop, not sure if this particular compilation check should just be disabled if it is buggy?

[slava77]

Failed Build

I found compilation warning when building: See details on the summary page.

I think this previous issue is a false positive from -Wstringop-overflow/-Warray-bounds by the way. The reported sizes don't match the actual StdArray types. I used a workaround to silence it, pulling the row refs out of the loop, not sure if this particular compilation check should just be disabled if it is buggy?

while the warning log is still alive
Is there a clear problem in the earlier implementation, which
essentially was pointing to the following being problematic

   for (unsigned int i = 0; i < t5Layers; ++i)
      pixelQuintuplets.logicalLayers()[pixelQuintupletIndex][2 + i] = x;
    for (unsigned int i = t5Layers; i < Params_T5::kLayers; ++i)
      pixelQuintuplets.logicalLayers()[pixelQuintupletIndex][2 + i] = 0;

IIUC, just placing these into one loop didn't work, only insertion of a ref auto& dstLayers = pixelQuintuplets.logicalLayers()[pixelQuintupletIndex]; to use it directly helped.

(full diff can be seen in :
https://github.com/cms-sw/cmssw/compare/1d072cde75f84b453ecc285009fbfc8ee7f1819f..de7d518f960258802cc957e5981507ddd627c820
)

@makortel @fwyzard @smuzaffar
does it look like a legitimate bad code to be fixed or a false-positive in the compiler?

[makortel]

-Wstringop-overflow/-Warray-bounds

We have seen false positives from these warnings before. I didn't (at least quickly) spot anything obviously wrong in the earlier version of the code.

[slava77]

-Wstringop-overflow/-Warray-bounds

We have seen false positives from these warnings before. I didn't (at least quickly) spot anything obviously wrong in the earlier version of the code.

Thanks for checking.
The code change related to this work around the false positive is compact/elegant enough that we could probably just move on.

@cms-sw/reconstruction-l2
please check this PR
Thank you.

[Moanwar]

+1

[cmsbuild]

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. @mandrenguyen, @sextonkennedy, @ftenchini (and backports should be raised in the release meeting by the corresponding L2)

[smuzaffar]

@makortel @fwyzard @smuzaffar
does it look like a legitimate bad code to be fixed or a false-positive in the compiler?

@makortel @slava77 , could it be that pixelQuintuplets.logicalLayers()[pixelQuintupletIndex] returns a temporary object? We also seen many array-bound warnings in GCC16 IBs (https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/el9_amd64_gcc16/www/tue/17.0-tue-23/CMSSW_17_0_X_2026-05-26-2300) . Instead of just assuming that those are false positive we should actually see it somewhere default constructor is called to create a temp object which is then unavailable when use it

[slava77]

@smuzaffar

@makortel @fwyzard @smuzaffar
does it look like a legitimate bad code to be fixed or a false-positive in the compiler?

@makortel @slava77 , could it be that pixelQuintuplets.logicalLayers()[pixelQuintupletIndex] returns a temporary object? We also seen many array-bound warnings in GCC16 IBs (https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/el9_amd64_gcc16/www/tue/17.0-tue-23/CMSSW_17_0_X_2026-05-26-2300) . Instead of just assuming that those are false positive we should actually see it somewhere default constructor is called to create a temp object which is then unavailable when use it

Are there example warnings in SoA access in GCC16?
I don't recognize the alpaka packages among those with warnings.

Looking for the one I know, in GCC16 in LSTCore log I don't see any warnings.
While in the release we have kind of the same pattern already

cmssw/RecoTracker/LSTCore/src/alpaka/TrackCandidate.h

Lines 42 to 43 in c3282cf

	CMS_UNROLL_LOOP for (int layerSlot = 0; layerSlot < Params_TC::kLayers; ++layerSlot) {
	candsExtended.logicalLayers()[trackCandidateIndex][layerSlot] = 0;

unclear if CMS_UNROLL_LOOP did some magic.

Is there a cleaner syntax to access/modify SoA elements?

[makortel]

@makortel @fwyzard @smuzaffar
does it look like a legitimate bad code to be fixed or a false-positive in the compiler?

@makortel @slava77 , could it be that pixelQuintuplets.logicalLayers()[pixelQuintupletIndex] returns a temporary object?

IIRC it does return a temporary, and I vaguely recall now we have seen warnings in similar situations before. I found #47634 and #45179

[mandrenguyen]

+1