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.

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

L1L2 TrackBuilder + FullMatchMemory fix #134

Merged
merged 3 commits into from
Mar 15, 2021
Merged

L1L2 TrackBuilder + FullMatchMemory fix #134

merged 3 commits into from
Mar 15, 2021

Conversation

aehart
Copy link
Contributor

@aehart aehart commented Feb 17, 2021

This is the first PR for the L1L2 TrackBuilder. The module passes C-simulation and C/RTL cosimulation using test-vectors generated with the aehart:purge_duplicates_test_vectors branch of the emulation. This branch will be merged into the main development branch soon, but for now, these test-vectors are downloaded as a supplemental tarball.

The module is pipelined with II=1 and the rewind option, and it also passes timing now according to the post-implementation report (clock period = 3.86 ns). The resource utilization is ~1.7% for LUTs and ~0.6% for FFs on a VU7P. This is a little high, but we can work on it in the future.

This PR also adds stub r to the FullMatch object, something that was accidentally omitted previously. This leads to an inconsistency between the output test-vectors of the MC, which do not have stub r, and the input test-vectors of the TB, which do have stub r. We get around this by introducing temporary template parameters, BARREL_FOR_MC and DISK_FOR_MC, which are used to select the versions of FullMatch that do not have stub r. After our next synchronization with the emulation, we will be able to remove these and just use the versions that have stub r.

I also took this opportunity to move the top functions of the TC into TrackletCalculatorTop.cc, just to be consistent with all the other modules.

@tomalin
Copy link
Contributor

tomalin commented Mar 1, 2021

Are the warnings in the vivado_hls.log for this all understood? e.g. "Ignore complete array partition directive", "improper streaming access", "Ignored multiple trip count directives", "Please consider removing the 'rewind' option", "This design is unable to schedule all read ports in the first II cycle. The RTL testbench may treat the design as non-pipelined".

@tomalin
Copy link
Contributor

tomalin commented Mar 1, 2021

Add comment to TrackFitMemory.h stating that it contains track + 4 barrel + 4 disk stubs. (Add update https://twiki.cern.ch/twiki/bin/view/CMS/HybridDataFormat#Track_Builder_output_format to say so). Twiki says format different for PS & 2S disk, but HLS code is same.

@tomalin
Copy link
Contributor

tomalin commented Mar 1, 2021

Could repetitive code in TrackFitMemory.h be eliminated with C arrays? e.g. Replace function "getLayer3Count()" by "getLayerCount(unsigned n)". And perhaps just use enum to set bit lengths/offsets of generic stub; and then use loop to set values of static const array elements kTFLayerCountMSB[n] for n = 0 to 3 etc.
This would also allow repetitive code to be eliminated from TrackBuilder.h, such as track.setStub2(...).

@tomalin tomalin changed the title L1L2 TrackBuilder L1L2 TrackBuilder + FullMatchMemory fix Mar 2, 2021
@tomalin
Copy link
Contributor

tomalin commented Mar 2, 2021

If FullMatchMemory should contain "r", please add it to https://twiki.cern.ch/twiki/bin/view/CMS/HybridDataFormat#FullMatch_written_by_MatchCalcul , and say if a PR correcting the C++ emulation is being submitted too?

@tomalin
Copy link
Contributor

tomalin commented Mar 8, 2021

Add comment to FullMatchMemory.h explaining what TCID means.

@aehart
Copy link
Contributor Author

aehart commented Mar 10, 2021

@tomalin Thanks for the comments. I think the code quality is much better after addressing them. Please find inline replies above and other replies below.

Are the warnings in the vivado_hls.log for this all understood? e.g. "Ignore complete array partition directive", "improper streaming access", "Ignored multiple trip count directives", "Please consider removing the 'rewind' option", "This design is unable to schedule all read ports in the first II cycle. The RTL testbench may treat the design as non-pipelined".

  • "Ignore complete array partition directive": removed by adjusting the parameters of the partitioning
  • "improper streaming access": One of the possible reasons given for this is "some entries are not used". Since we cannot know ahead of time how many tracks will be output for a given event, I think this warning is therefore unavoidable. It should also be safe to ignore, because the corresponding interfaces are synthesized as FIFOs of depth one, and the C arrays just have to be large enough to accommodate the maximum number of tracks.
  • "Ignored multiple trip count directives": I have no idea what this means. I can make it disappear if I introduce the loop_tripcount pragma to the full_match loop:
…
  full_matches : for (unsigned short i = 0; i < kMaxProc; i++) {
#pragma HLS pipeline II=1 rewind
#pragma HLS loop_tripcount min=108 max=108
…

However, since this appears to have no effect on the resulting module, and since there is no obvious way to do this without introducing 108 as a magic number, I think we can and should ignore this warning.

  • "Unable to schedule…": This appears for many of the modules, and it will require investigation in an HDL test bench. I suspect it means that we will need a gap of some number of clock cycles between the start of the last iteration of one event and the start of the first iteration of the next event. But right now, that's just a guess.

Add comment to TrackFitMemory.h stating that it contains track + 4 barrel + 4 disk stubs. (Add update https://twiki.cern.ch/twiki/bin/view/CMS/HybridDataFormat#Track_Builder_output_format to say so). Twiki says format different for PS & 2S disk, but HLS code is same.

Done.

Could repetitive code in TrackFitMemory.h be eliminated with C arrays? e.g. Replace function "getLayer3Count()" by "getLayerCount(unsigned n)". And perhaps just use enum to set bit lengths/offsets of generic stub; and then use loop to set values of static const array elements kTFLayerCountMSB[n] for n = 0 to 3 etc.
This would also allow repetitive code to be eliminated from TrackBuilder.h, such as track.setStub2(...).

I now calculate layer/disk-dependent LSBs and MSBs using constexpr functions, which I put in an intermediate class, TrackFitBits, between TrackFitBase and TrackFit. And in the relevant methods of TrackFit, I index the hit number with a template parameter.

If FullMatchMemory should contain "r", please add it to https://twiki.cern.ch/twiki/bin/view/CMS/HybridDataFormat#FullMatch_written_by_MatchCalcul , and say if a PR correcting the C++ emulation is being submitted too?

The TWiki has been updated, and the emulation was updated with PR #70.

Add comment to FullMatchMemory.h explaining what TCID means.

Done.

@tomalin
Copy link
Contributor

tomalin commented Mar 14, 2021

We should perhaps by default only print "reference vs computed" from compareMemWithFile() for events where they are inconsistent, or for the first or last 10 events. Currently, test-bench prints 8000 lines of comparisons, all of which show perfect agreement.

Copy link
Contributor

@tomalin tomalin left a comment

Choose a reason for hiding this comment

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

Looks good. Great to see a new module.

@aehart aehart merged commit e5bea11 into master Mar 15, 2021
@aehart aehart deleted the track_builder branch March 15, 2021 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants