-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
L1 emulator for LLP displaced jet trigger (L1 6:1 LUT, L2 jet algo), L1TNtuples addition of jet hwQual #36919
Conversation
…dition of jet hwQual, fix comment in HcalFinegrainBit.cc
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36919/28230
|
A new Pull Request was created by @gk199 (Gillian Kopp) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
-1 Failed Tests: Build ClangBuild BuildI found compilation warning when building: See details on the summary page. Clang BuildI found compilation warning while trying to compile with clang. Command used:
See details on the summary page. |
…t for LLP trigger logic)
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36919/28248
|
Pull request #36919 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
please test |
L1T DQM histogram comparison comments:
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cc9948/22463/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
+1 |
We're getting ASAN heap-buffer-overflow errors, likely related to this PR, see #37012 |
@@ -107,9 +107,14 @@ namespace l1t { | |||
|
|||
HcalTrigTowerDetId id(cEta, cPhi); | |||
const auto tp = hcalTPGs->find(id); | |||
|
|||
int fg_bits = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint32_t fg_bits = 0
|
||
int fg_bits = 0; | ||
for (int index = 0; index < 6; index++) | ||
fg_bits += tp->SOI_fineGrain(index) << index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fg_bits |= tp->SOI_fineGrain(index) << index;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop needs to be inside this conditional:
if (tp != hcalTPGs->end()) {
I added some suggestions above in |
Yes, you can check the latest ASAN build from the IB dashboard and run a failing workflow there, e.g.
|
@gk199 Could you check if your suggested fixes above remove the errors? |
I tested my suggested fix, and still have the heap-buffer-overflow issue. I also tested a number of other modifications, such as doing the 6:1 logic in CaloLayer1Packer.cc prior to passing through setFB instead of doing the 6:1 logic in UCTCTP7RawData.h. Any other suggestions or guidance will be much appreciated as I work to resolve this. |
This loop: cmssw/EventFilter/L1TRawToDigi/plugins/implementations_stage2/CaloLayer1Packer.cc Lines 111 to 113 in 62d26a2
needs to be inside this conditional: cmssw/EventFilter/L1TRawToDigi/plugins/implementations_stage2/CaloLayer1Packer.cc Lines 115 to 118 in 62d26a2
As is, when id isn't found it's indexing off the end of the vector.
|
Many thanks @dan131riley for the fix and the explanation! I confirm this resolves the step2 heap-buffer-overflow from However, I see an error in step3, which seems unrelated to any code in this PR, as it is part of
As this is an unrelated file to this L1 emulator PR, may I now open a new PR to fix the heap-buffer-overflow with the changes in |
@gk199 please make a new PR with the step2 fix. The fix is clearly correct and needed, the step3 error can be dealt with separately. |
L1T ASAN heap-buffer-overflow fix resulting from PR #36919 (12_4_X)
L1T ASAN heap-buffer-overflow fix resulting from PR #36919
PR description:
This PR implements the L1 emulator of the LLP displaced jet trigger algorithm. Calo L1 takes the 6 fine grain bits from the HCAL uHTR (PRs #34600 and #35599) and uses a 6:1 LUT to set a single bit of tower hwQual based on the logical OR of the timing and depth information. The Calo L2 jet algorithm flags a jet as delayed if there are >= 2 flagged towers within the 9x9 jet region. This is saved in the LSB of jet.hwQual, 1 if jet is flagged as containing an LLP, 0 otherwise. L1 and L2 firmware updates have been completed.
The uGT emulator follows, and is implemented in recent PR #36868. L1 Menu Tools is in the process of being updated to include the displaced jet bit, in PR #74.
The L1 seed JIRA ticket, including slides detailing the testing of the L1 emulator and links to relevant presentations, is here. Slides 2-10 focus on the L1 emulator implementation and validation tests.
The emulator implementation has been presented at L1 Trigger CMS Week and to the HCAL Trigger group.
Additionally, the L1TNtuples are updated to store the jet hwQual information (in
L1AnalysisL1Upgrade.cc
andL1AnalysisL1UpgradeDataFormat.h
). Tested by myself and @elfontan.A single comment in
HcalFinegrainBit.cc
is updated to reflect the correct assignment of the 6 fine grain bits from the uHTR.PR validation:
Passed runTheMatrix.py tests. Tested in CMSSW_12_3_X_2022-02-08-1100.