Skip to content

Conversation

@mcoshiro
Copy link
Contributor

Large collection of developments for EMP-integrated project. This branch now generates and meets timing fairly reliably (~75% of the time for more performant implementation strategies). Hardware usage is much more reliable, and most events agree between simulation and hardware, though there seem to be edge cases where simulation and hardware disagree, namely the first event in simulation seems to be special and the last event fed through the buffer on hardware always seems to differ. There are also various bug fixes in the inter-FPGA interface, the TB-KFin/KF interface, and in KFout.

Changes include:

  • Updates for compatibility with EMP 0.9.X
  • Updates for compatibility with other changes (timing improvements, 8->2 TBs)
  • Bug fix for multithreaded use of download.sh
  • Add pipelining for FPGA1 input links
  • Bug fix for FPGA1 output link ordering
  • Changes to FPGA2 input for more/better pipelining
  • Automatic resyncing of FPGA2 SectorProcessor. sp2_mem_writer now monitors BX change, and will reset SectorProcessor if it is not in time with start signal
  • Updates in pattern reco-fitting interface to fix bugs related to merging streams. The TB output is now split by seed type into 8 streams, which are fed to KFin. Merging is then performed between KFin and KF
  • Bug fix for KFout routing module logic (this is deprecated, but this branch still uses the old version)
  • Soft floorplans for both FPGAs (previously hard floorplans included, but commented out/unused. Would prefer to keep for at least 1 commit to master for the record if other branches are deleted)
  • Entire new collection of python scripts for processing EMP-formatted data and comparing between emulation, simulation, and hardware

Things that are currently included, but may be modified/removed

  • HLS target clock frequency set to 400 MHz
  • FPGA2 generates with 2 pipelining levels

This branch is currently based on memory_resync.

@mcoshiro mcoshiro force-pushed the emp_agreement_updates_rebase branch from 0127a69 to 4e0719e Compare July 1, 2025 13:14
@mcoshiro mcoshiro force-pushed the emp_agreement_updates_rebase branch from 4e0719e to 4847bfc Compare August 31, 2025 14:39
@mcoshiro
Copy link
Contributor Author

Turns out the original implementation of tf_to_kf was okay, so that does not need updating in this PR. I've locally rebased on master, but gitlab is behind a firewall over this weekend, so I guess I'll try building the FW and testing on Monday

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcoshiro Since this script and fpga2_runipsynth.tcl don't seem to be called from any other script, what are their use cases? Also, I think they could be made more generic, e.g.:

# Minimal script to synthesize all IPs

set origin_dir "."
set proj_name "tf_1"
open_project $origin_dir/$proj_name.xpr
update_compile_order -fileset sources_1

# Create and launch all OOC runs for all IPs
launch_runs synth_1 -scripts_only
reset_runs [get_runs *_synth_1]
launch_runs -jobs 24 [get_runs *_synth_1]
wait_on_runs [get_runs *_synth_1]

exit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I hadn't realized you could clean this up with the wildcards. This is just a small utility I've been using to speed things up since running the various synth runs from the GUI is really slow for some reason; I wanted to put it somewhere version controlled so it doesn't get lost, but I can remove it from this repo and store it somewhere else if you would like.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's what I suspected, and I've written similar things in the past. I think this is a useful tool to have in the repo, and I would just propose making it more generic, so that we only need one script that can be used for both the standalone and EMP-integrated projects, as well as moving it one directory up, i.e., to IntegrationTests/common/script/. Being more generic, I think that location makes more sense.

@mcoshiro mcoshiro force-pushed the emp_agreement_updates_rebase branch from 4847bfc to df1e923 Compare September 21, 2025 14:24
@mcoshiro
Copy link
Contributor Author

Hm, something weird is going on: since rebasing on the current master, sometimes, the FPGA2 SectorProcessor gets built in the CombinedConfig_FPGA1/hdl directory. At first, I thought maybe this was caused by some weird thing that was cached locally, but this appears to have happened to the gitlab CI as well...

@aehart
Copy link
Contributor

aehart commented Sep 23, 2025

There were some strange errors in https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/61896820, so I cleaned out some directories on the Gitlab runner and tried rerunning the CI tests. Now, the job that runs the simulation passes, but there are many discrepancies when the results are checked:
https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/61925734

The only thing I can imagine having this effect are the changes to emData/download.sh in 4ed5130, where the delay argument is updated. Maybe this means some of the updates to the test bench that @aryd made need to be retuned? What do you think, @mcoshiro?

@aehart
Copy link
Contributor

aehart commented Sep 23, 2025

There were some strange errors in https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/61896820, so I cleaned out some directories on the Gitlab runner and tried rerunning the CI tests. Now, the job that runs the simulation passes, but there are many discrepancies when the results are checked: https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/jobs/61925734

The only thing I can imagine having this effect are the changes to emData/download.sh in 4ed5130, where the delay argument is updated. Maybe this means some of the updates to the test bench that @aryd made need to be retuned? What do you think, @mcoshiro?

Never mind. The update to download.sh only affects the full FPGA2 project, not the reduced FPGA1 project. So I don't know why this is failing here.

@mcoshiro
Copy link
Contributor Author

Hm, I locally have a build of firmware-hls's master as well as this branch, so I can see if I can track down the difference

@aehart
Copy link
Contributor

aehart commented Sep 23, 2025

Hm, I locally have a build of firmware-hls's master as well as this branch, so I can see if I can track down the difference

Actually, I think I just found the issue, and it's this commit: fe772a9. I know we've gone back and forth with this previously, but emData/MemPrints no longer exists after download.sh runs, so I think this commit needs to be reverted.

@mcoshiro
Copy link
Contributor Author

Hm, I locally have a build of firmware-hls's master as well as this branch, so I can see if I can track down the difference

Actually, I think I just found the issue, and it's this commit: fe772a9. I know we've gone back and forth with this previously, but emData/MemPrints no longer exists after download.sh runs, so I think this commit needs to be reverted.

Can do, but if I recall, not having this check previously resulted in some of the python scripts stepping on each other when the build was run with multiple threads. Was this fixed somewhere else?

@aehart
Copy link
Contributor

aehart commented Sep 23, 2025

Hm, I locally have a build of firmware-hls's master as well as this branch, so I can see if I can track down the difference

Actually, I think I just found the issue, and it's this commit: fe772a9. I know we've gone back and forth with this previously, but emData/MemPrints no longer exists after download.sh runs, so I think this commit needs to be reverted.

Can do, but if I recall, not having this check previously resulted in some of the python scripts stepping on each other when the build was run with multiple threads. Was this fixed somewhere else?

We discussed this just now during the meeting, but yes, this was fixed in this commit that was part of #363: 416a056

Sorry for burying it in a somewhat unrelated PR.

@aehart
Copy link
Contributor

aehart commented Sep 24, 2025

Thanks for reverting the change in download.sh, @mcoshiro. Apparently, we still see discrepancies in the reduced FPGA1 simulation, and I realized the change to the target clock frequency here may be causing issues:

# Set clock frequency
create_clock -period 400MHz -name default

I think changing the target frequency changes the latency of some modules, which throws off the printout from the test bench. I tested this by creating a new branch that's identical to this PR branch, but where I revert the target frequency (michael_pr_test). And it passes all the CI tests:
https://gitlab.cern.ch/cms-l1tk/firmware-hls/-/pipelines/12947348

We want to be able to adjust the target frequency in order to help with timing, so I would propose rebasing once more onto the master branch after #373 is merged. Hopefully, the printout will then be flexible enough to not be thrown off by modules having different latencies.

@aehart
Copy link
Contributor

aehart commented Oct 1, 2025

@mcoshiro Now that #373 has been merged, could you please try rebasing, so we can see if that helps the discrepancies in the CI?

@mcoshiro mcoshiro force-pushed the emp_agreement_updates_rebase branch from 10d6e9b to ff314ca Compare October 2, 2025 20:14
Copy link
Contributor

@aehart aehart left a comment

Choose a reason for hiding this comment

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

I investigated the discrepancies for the reduced FPGA1 project that we see even after rebasing on #373, and found they were caused by inconsistent latencies in the InputRouters, something we've seen before. Apparently, increasing the target frequency used for C-synthesis not only increases the latencies for the IRs, but also makes the latencies different for different instances, for some reason.

I took the liberty of pushing a commit that adds a latency pragma to the IR, which fixed the issue in my testing. And I'll set this PR to merge automatically once the CI tests finish.

@aehart aehart enabled auto-merge (squash) October 6, 2025 13:01
@mcoshiro
Copy link
Contributor Author

mcoshiro commented Oct 6, 2025

I investigated the discrepancies for the reduced FPGA1 project that we see even after rebasing on #373, and found they were caused by inconsistent latencies in the InputRouters, something we've seen before. Apparently, increasing the target frequency used for C-synthesis not only increases the latencies for the IRs, but also makes the latencies different for different instances, for some reason.

I took the liberty of pushing a commit that adds a latency pragma to the IR, which fixed the issue in my testing. And I'll set this PR to merge automatically once the CI tests finish.

I haven't had a lot of cycles for debugging this PR, so thanks for looking into this Andrew! I guess this also explains (some of) why some projects seemed to have the variable IR latency thing and some didn't.

@aehart aehart merged commit 1391ccb into master Oct 6, 2025
1 check passed
@aehart aehart deleted the emp_agreement_updates_rebase branch October 6, 2025 16:23
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.

3 participants