[Prepacker] Removed Valid Flag From Molecules #2796
Conversation
github-actions
bot
added
VPR
|
@vaughnbetz This has passed CI. Please review and merge when you have a moment. This removed the "valid" flag within the pack molecules which really should be part of the Cluster Legalizer object. |
vpr/src/pack/cluster_legalizer.cpp
Outdated
| @@ -997,11 +997,70 @@ static void update_molecule_chain_info(t_pack_molecule* chain_molecule, const t_ | |||
| VTR_ASSERT(false); | |||
| } | |||
|
|
|||
| /** | |||
| * @brief Revalidate the molecules associated with this pb. The mol_validated | |||
There was a problem hiding this comment.
If you know anything more about what this routine does (how does it record that molecules are valid? what does it do if mol_validated is true?) it would be good to expand this comment. If you don't know (you're just moving the code), then we can leave the comment as is.
There was a problem hiding this comment.
I actually knew very little about this routine. In all honesty it made no sense. I added the "mol_validated" flag as a hot fix to keep the behaviour exactly the same. I agree that it was a rough fix. I decided to dig into this method in detail and found that it was completely not necessary anymore (given my recent changes). I greatly simplified the method and turned it into a "reset_molecule_info" method, which does basically the same thing.
| } | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * @brief Revert trial atom block iblock and free up memory space accordingly. | ||
| */ |
There was a problem hiding this comment.
Good to expand this comment with as much as you know; you should at least know what mol_validated does so it could be commented.
There was a problem hiding this comment.
See previous response. I agree that the mol_validated variable was confusing and reworked the function to make it better.
AlexandreSinger
force-pushed
the
feature-prepacker-rework
branch
from
49f3ab3
to
55f3ef1
Compare
vpr/src/pack/cluster_legalizer.cpp
Outdated
| static void reset_molecule_info(t_pack_molecule* mol) { | ||
| // when invalidating a molecule check if it's a chain molecule | ||
| // that is part of a long chain. If so, check if this molecule | ||
| // have modified the chain_id value based on the stale packing |
There was a problem hiding this comment.
Want to add a TODO to say this code may be removable?
Pack molecules originally contained a flag called "valid" which signified that the molecule has not been clustered yet. This flag is not necessary since the Cluster Legalizer maintains this information internally. Removed the valid flag from the pack molecule struct. See issue verilog-to-routing#2791
Found that the revalid molecules method was more complicated than it needed to be in the context of the Cluster Legalizer. Simplified its code so that it makes more sense.
AlexandreSinger
force-pushed
the
feature-prepacker-rework
branch
from
55f3ef1
to
a34ebf1
Compare
Pack molecules originally contained a flag called "valid" which signified that the molecule has not been clustered yet. This flag is not necessary since the Cluster Legalizer maintains this information internally. Removed the valid flag from the pack molecule struct.
See issue #2791