[WIP] Reorganize place timing files #2829
Conversation
…neighboring_average
github-actions
bot
added
VPR
github-actions
bot
added
the
libarchfpga
it doesn't compile
| std::shared_ptr<const SetupTimingInfo> timing_info) | ||
| : clb_nlist_(clb_nlist) | ||
| , pin_lookup_(netlist_pin_lookup) | ||
| , timing_info_(std::move(timing_info)) |
There was a problem hiding this comment.
I am a little confused by the use of a shared pointer here. And I especially find this move a bit strange. Why not just pass a const reference to the SetupTimingInfo object? Do you think the original pointer may get destroyed during the lifetime of this class? The shared pointer could make sense if the owner of the timing_info object may be destroyed before the PlacerCriticalities object is finished.
There was a problem hiding this comment.
Looks generally good. A few suggestions on commenting.
Also, src/place/timing/delay/.cpp seems like an excessively long path name.
I suggest src/place/delay/.cpp instead; there's no real reason delay has to go under timing, and I think code is harder to discovery when the directory hierarchy gets too deep.
| ClusterBlockId b_from = cluster_ctx.clb_nlist.net_driver_block(crit_pin.first); | ||
|
|
||
| //Check if picked block type matches with the blk_type specified, and it is not fixed | ||
| //blk_type from propose move doesn't account for the EMPTY type | ||
| auto b_from_type = cluster_ctx.clb_nlist.block_type(b_from); | ||
| if (b_from_type->index == logical_blk_type_index) { | ||
| if (b_from_type->index == logical_blk_type_index || logical_blk_type_index < 0) { |
There was a problem hiding this comment.
I suggest adding a comment that explains what this if is doing and what logical_blk_type_index < 0 means.
| * @param logical_blk_type_index: the agent type of the moving block. | ||
| * @param logical_blk_type_index The logical type of the moving block. If a negative value is passed, | ||
| * the block is selected randomly from all movable blocks and not from a specific type. | ||
| * @param rng A random number generator used to select a random highly critical block. | ||
| * |
There was a problem hiding this comment.
Add Doxygen comments for the missing parameters, or at least the ones you understand. placer_criticalities is newly added so it at least should be easy to comment.
| * returned by SetupTimingInfo. However, the set returned only reflects the connections | ||
| * changed by the last call to the timing info update. | ||
| * | ||
| * Therefore, if SetupTimingInfo is updated twice in succession without criticalities |
There was a problem hiding this comment.
I don't find this paragraph easy to understand ... if you understand it, please take a crack at clarifying it.
| public: //Lifetime | ||
|
|
||
| ///@brief Allocates space for the timing_place_crit_ data structure. | ||
| PlacerCriticalities(const ClusteredNetlist& clb_nlist, |
There was a problem hiding this comment.
Should explain the parameters here, particularly why the timing_info is a shared pointer (what is the usage/lifetime model)?
| } | ||
|
|
||
| /** | ||
| * @brief Updated the setup slacks in the timing_place_setup_slacks_ data structure. |
There was a problem hiding this comment.
Shouldn't this comment be in the .h file? Suggest moving it there, or deleting it if it is a duplicate.
| * | ||
| * Should consistently call this method after the most recent timing analysis to | ||
| * keep the setup slacks stored in this class in sync with the timing analyzer. | ||
| * If out of sync, then the setup slacks cannot be incrementally updated on |
There was a problem hiding this comment.
Merge in the extra information from the .cpp file comment:
- @brief Updated the setup slacks in the timing_place_setup_slacks_ data structure.
- If the setup slacks are not updated immediately after each time we call
- timing_info->update(), then timing_info->pins_with_modified_setup_slack()
- cannot accurately account for all the pins that need to be updated.
- In this case,
recompute_requiredwould be true, and we update all setup slacks - from scratch.
vpr/src/placedirectory has too many files, making it difficult to locate related files. This PR organizes timing related files and classes under a new directoryvpr/src/place/timing.place/timing/delay_model.timing_place.cpp/.hand created seperate files forPlacerCriticalities,PlacerSetupSlacks, andPlacerTimingCosts.physical_types_util.cppare now implemented as member functions of structs declared inphysical_types.h.