dfflibmap: pass selection to dfflegalize#5729
dfflibmap: pass selection to dfflegalize#5729abhinavputhran wants to merge 1 commit intoYosysHQ:mainfrom
Conversation
…alize on the whole design regardless of the active selection, causing unselected modules to be modified. Fix by appending selected module names to the dfflegalize command. Fixes YosysHQ#5650
|
The included tests pass even without this change (and that's not even getting into whether the change is the correct way to do it or not). Will post more in the issue thread, but using |
| select -module top_unmapped -assert-none t:dffn | ||
| select -module top_unmapped -assert-none t:dffe | ||
| select -module top_unmapped -assert-none t:dffsr |
There was a problem hiding this comment.
Test dfflegalize isn't running, not dfflibmap.
| select -module top_unmapped -assert-none t:dffn | |
| select -module top_unmapped -assert-none t:dffe | |
| select -module top_unmapped -assert-none t:dffsr | |
| select -module top_unmapped -assert-count 1 t:$_DFFE_PP_ | |
| select -module top_unmapped -assert-none t:$_DFF_N_ |
| for (auto module : design->selected_modules()) | ||
| dfflegalize_cmd += " " + module -> name.str(); |
There was a problem hiding this comment.
This ain't it. Not only does this expand partially selected modules into fully selected modules, but it is still selecting all t:$_DFF* t:$_SDFF*, making this actively worse and causing failures if the design contains any other types of flop (raising an error in dfflegalize).
My current thought is running through all the currently selected cells, pushing them to a new selection if they are the right type, and then using Pass::call_on_selection() with the new selection. Or doing % t:$_DFF* t:$_SDFF* %u %i might work, but that's starting to get messy in terms of readability.
|
@abhinavputhran , when you are adding tests for a fix they should fail without the fix. My usual approach/suggestion is to write the tests before writing the fix, to make sure you understand what isn't working. This is the second PR where you've added tests that don't show anything. I suggest you take another look at your third PR and make sure the test is actually useful (at a very brief glance, just calling |
|
Well noted, first time writing tests so I'm learning. I'll make sure to verify each test fails without the fix before submitting and that the tests test what they should |
|
Thanks :) |
3 participants
Fixes #5650
dfflibmap was calling dfflegalize on the whole design regardless of the active selection, causing unselected modules to be modified unexpectedly.
Fix by appending the selected module names to the dfflegalize command string.
Added a test to tests/techmap/dfflibmap.ys that verifies unselected modules are not modified after calling dfflibmap with a selection.