Correlator: support for running barrel TM18 #1187
Conversation
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. Attempts to compile this PR succeeded!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found no issues with the code checks!
I found no issues with the headers!
|
|
Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation. I found 1 files that did not meet formatting requirements:
Please run
|
|
Hello triggerDoctor, I can fix it but it's not in my package so it must have entered in another PR. Giovanni
|
aloeliger
added
Phase-2
L1Trigger/Phase2L1ParticleFlow/interface/regionizer/multififo_regionizer_elements_ref.h
Outdated
Show resolved
Hide resolved
| } else { | ||
| // uncommenting the message below may be useful for debugging | ||
| //dbgCout() << "WARNING: sector buffer is full for " << typeid(T).name() << ", pt = " << t.intPt() | ||
| // << ", eta = " << t.intEta() << ", phi = " << t.intPhi() << "\n"; | ||
| } |
There was a problem hiding this comment.
Is this debug still necessary? If so could you remove the commented stuff and put in LogDebug instead?
There was a problem hiding this comment.
dbgCout() gets compiled to the CMS MessageLogger when compiled within CMSSW, and to cout when compiled standalone (this emulator is also used from within Vitis HLS testbenches, where the MessageLogger is not avaiable).
I'd prefer to keep it as is; this code was already in CMSSW ( https://github.com/cms-sw/cmssw/blob/master/L1Trigger/Phase2L1ParticleFlow/interface/regionizer/buffered_folded_multififo_regionizer_ref.h#L128-L130 ), I just refactored to a different place.
| bool isGood = | ||
| (nfifos == 1 || nfifos == 2 || nfifos == 3 || nfifos == 4 || nfifos == 6 || nfifos == 8 || nfifos == 12); | ||
| if (!isGood) { | ||
| dbgCerr() << "Error, created regionizer for nfifos == " << nfifos << ", not supported." << std::endl; |
There was a problem hiding this comment.
If this isn't one of the usual CMSSW Logging streams, could this be removed in favor of LogError?
There was a problem hiding this comment.
as above, dbgCerr() goes to MessageLogger. See https://github.com/cms-sw/cmssw/blob/master/L1Trigger/Phase2L1ParticleFlow/interface/dbgPrintf.h
| assert(!init_); | ||
| init_ = true; | ||
| assert(out.size() == nregions_post_); |
There was a problem hiding this comment.
Since the amount of asserts here is relatively low, it would be probably better to throw cms::Exceptions which can have some more information, than just an assert, which will only mention the line number of the failed assertion on failure.
There was a problem hiding this comment.
I can't use cms::Exception's since this code is also compiled outside of CMSSW in Vitis HLS testbenches.
I could use std exceptions, but eventually these asserts are not supposed to ever trigger when the code is running, they're there just to catch mistakes during development and to inform the reader of the assumptions that this piece of code is doing, so there's not much gain in making them more verbose.
L1Trigger/Phase2L1ParticleFlow/src/regionizer/middle_buffer_multififo_regionizer_ref.cpp
Outdated
Show resolved
Hide resolved
|
Alright, I think I'm good here. @epalencia Anything from your side? If not, @gpetruc Could you go ahead and open a PR for this to the main CMSSW if there isn't one already? |
|
Nothing from my side. @gpetruc , please go ahead and open the PR in master. |
gpetruc
force-pushed
the
barrel_tm18_133x
branch
from
5833f9d
to
67aca4f
Compare
|
Hi, I noticed that when removing the debug printout I had introduced a formatting issue, so now I've re-run code-format and updated the last commit.
|
|
@gpetruc There's a number of accumulated formatting issues that I will likely fix by hand when your PR is merged. Don't worry about it for now. The IB is sort of a rough draft/best effort sort of thing after we've changed up how we use it. |
|
Hi, |
|
@gpetruc , ORPs signature is still not there yet, would you mind to wait a bit for it or you need it already in the IB? |
|
I can wait a few dayxs, but eventually I'd like to have it in the IB since there's a large firmware PR that I can't merge until this one is merged. |
|
Ok, if main PR is not merged in master tomorrow afternoon, I'll merge it here. |
epalencia
merged commit 4bbec07
into
cms-l1t-offline:phase2-l1t-integration-13_3_0_pre3
|
Tagged as |
This PR extends the emulators of the correlator layer 1 to support running the barrel at tmux 18 (albeit for now only on serenity boards)
There are no changes in the default correlator layer 1 configuration that is used in the L1 menu, which runs an idealized version of the barrel at tmux6.
Having the tmux18 version in CMSSW is however useful to do studies of this possible architecture and prepare pattern files for single and multi board tests at b904.
If this looks ok to you, I'll make the corresponding PR to CMSSW master this week
@cerminar