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.

Sign up for GitHub

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

Jump to bottom

fix c++20 compilation errors in L1Trigger/TrackFindingTracklet #43673

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

fix c++20 compilation errors in L1Trigger/TrackFindingTracklet #43673

wants to merge 2 commits into from

Conversation

dan131riley
Copy link

PR description:

In c++20, std::accumulate was changed so that the type of the initial value must be an rvalue and the preferred signature for the binary operation is Ret fun(const Type1 &a, const Type2 &b);. In a couple of places in L1Trigger/TrackFindingTracklet, the binary op was modifying the running value argument, which isn't necessary, doesn't conform to the preferred function signature, and breaks in c++20 because it isn't an rvalue. This PR fixes those compilation problems in the CPP20 builds and removes a few C-style casts.

In addition, in LTO builds we get a warning

In function '_S_ref',
    inlined from 'operator[]' at /data/cmsbld/jenkins/workspace/build-any-ib/w/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/array:219:25,
    inlined from 'benddecode' at /data/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/d190cc84d43df9162e76772777821e0e/opt/cmssw/el8_amd64_gcc12/cms/cmssw/CMSSW_14_0_X_2024-01-01-2300/src/L1Trigger/TrackFindingTracklet/interface/Settings.h:446:42,
    inlined from 'initBendMatch' at /data/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/d190cc84d43df9162e76772777821e0e/opt/cmssw/el8_amd64_gcc12/cms/cmssw/CMSSW_14_0_X_2024-01-01-2300/src/L1Trigger/TrackFindingTracklet/src/TrackletLUT.cc:955:42:
  /data/cmsbld/jenkins/workspace/build-any-ib/w/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/include/c++/12.3.1/array:61:36: warning: array subscript 18446744071562067968 is above array bounds of 'const struct array[16]' [-Warray-bounds]
    61 |       { return const_cast<_Tp&>(__t[__n]); }
      |                                    ^
/data/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/d190cc84d43df9162e76772777821e0e/opt/cmssw/el8_amd64_gcc12/cms/cmssw/CMSSW_14_0_X_2024-01-01-2300/src/L1Trigger/TrackFindingTracklet/interface/TrackletLUT.h: In member function 'initBendMatch':
/data/cmsbld/jenkins/workspace/build-any-ib/w/tmp/BUILDROOT/d190cc84d43df9162e76772777821e0e/opt/cmssw/el8_amd64_gcc12/cms/cmssw/CMSSW_14_0_X_2024-01-01-2300/src/L1Trigger/TrackFindingTracklet/interface/TrackletLUT.h:120:21: note: while referencing 'settings_'
  120 |     const Settings& settings_;
      |                     ^

This appears to be a false positive, where the LTO optimizer is concluding that it's possible a -1 could be passed to an unsigned and thus interpreted as a very large value. This PR adds an assertion that this won't happen, which quiets the LTO warning.

PR validation:

Purely technical fix. Compiles, should have no effect on performance. A limited matrix was run to verify that the new assertion doesn't fire in normal workflows.

@cmsbuild cmsbuild added this to the CMSSW_14_0_X milestone Jan 6, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2024 •
edited
Loading

cms-bot internal usage

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2024

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43673/38340

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

@dan131riley dan131riley mentioned this pull request Jan 6, 2024
Merged
@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2024

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43673/38341

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 6, 2024

A new Pull Request was created by @dan131riley (Dan Riley) for master.

It involves the following packages:

  • L1Trigger/TrackFindingTracklet (l1)

@epalencia, @cmsbuild, @aloeliger can you please review it and eventually sign? Thanks.
@missirol, @skinnari, @erikbutz, @Martin-Grunewald this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

aloeliger
aloeliger reviewed Jan 7, 2024
View reviewed changes
L1Trigger/TrackFindingTracklet/src/TrackletLUT.cc
@@ -901,6 +901,7 @@ void TrackletLUT::initProjectionDiskRadius(int nrbits) {
void TrackletLUT::initBendMatch(unsigned int layerdisk) {
unsigned int nrinv = NRINVBITS;
double rinvhalf = 0.5 * ((1 << nrinv) - 1);
assert(layerdisk < 16); // should be a named constant for this
Copy link
Contributor

Choose a reason for hiding this comment

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

Not urgent from my side, but if we know this is a bit "magic number"-ish, should we just go ahead and make this a constant?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am far from being an expert of this code, but that 16 to me looks like the total number of layer/disks in the phase2 layout. I.e. 2 * N_DISK + N_LAYER:

cmssw/L1Trigger/TrackFindingTracklet/interface/Settings.h

Lines 25 to 26 in c1580c8

constexpr int N_LAYER = 6; // # of barrel layers assumed
constexpr int N_DISK = 5; // # of endcap disks assumed

@makortel makortel mentioned this pull request Jan 8, 2024
Open
@dan131riley dan131riley marked this pull request as draft January 8, 2024 19:37
@dan131riley
Copy link
Author

This PR will be updated to resolve the conflicts with @Dr15Jones #43601. Title and description will be updated.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 6, 2024

Milestone for this pull request has been moved to CMSSW_14_1_X. Please open a backport if it should also go in to CMSSW_14_0_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_0_X, CMSSW_14_1_X Feb 6, 2024
@aloeliger
Copy link
Contributor

@dan131riley Any progress on this?

@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_14_2_X. Please open a backport if it should also go in to CMSSW_14_1_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Aug 27, 2024
@antoniovilela
Copy link
Contributor

ping (to make bot change milestone)

@cmsbuild cmsbuild modified the milestones: CMSSW_14_1_X, CMSSW_14_2_X Sep 3, 2024
@cmsbuild cmsbuild mentioned this pull request Oct 12, 2024
Merged
@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_15_0_X. Please open a backport if it should also go in to CMSSW_14_2_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_2_X, CMSSW_15_0_X Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloeliger aloeliger aloeliger left review comments

@mmusich mmusich mmusich left review comments

Assignees
No one assigned
Labels
code-checks-approved l1-pending orp-pending pending-signatures tests-pending
Projects
None yet
Milestone
CMSSW_15_0_X
Development

Successfully merging this pull request may close these issues.
5 participants
@dan131riley @cmsbuild @aloeliger @antoniovilela @mmusich