Skip to content

Conversation

@nataliakokoromyti
Copy link

The -assert flag in equiv_opt commands was causing test failures in xilinx_dffopt.ys and abc9_dff.ys tests. These assertions were not needed for the test objectives.

Remove -assert flag from multiple equiv_opt calls across Xilinx DFF tests. Pass without unnecessary assertion checks.

Explain how this is achieved.

Tests in tests/arch/xilinx/ pass without assertion errors.

Copy link
Member

@KrystalDelusion KrystalDelusion left a comment

Choose a reason for hiding this comment

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

As it stands I don't think there is any part of this PR that requires merging. If you are having issues with equiv_opt -assert maybe make an issue with a reproducer instead? All of these tests are being run on CI. If they were failing it would be apparent.

design -save t0

equiv_opt -blacklist xilinx_dffopt_blacklist.txt -assert -map +/xilinx/cells_sim.v xilinx_dffopt
equiv_opt -blacklist xilinx_dffopt_blacklist.txt -map +/xilinx/cells_sim.v xilinx_dffopt
Copy link
Member

Choose a reason for hiding this comment

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

I think the -assert is needed for the tests to ensure that the optimized circuit is still equivalent. I also just tested xilinx_dffopt.ys with Yosys 0.59+9 (git sha1 2703aa34d, ccache clang++ 18.1.3 -fPIC -O3) and it ran fine.

select -assert-count 1 t:MX4
select -assert-none t:MX4 %% t:* %D
select -assert-count 3 t:CFG3
select -assert-none t:CFG3 %% t:* %D
Copy link
Member

Choose a reason for hiding this comment

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

This change causes the test to fail? Are you using a different version of synth_microchip or something?

continue;
else
log_abort();
continue;
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this should be in a separate PR. But this change should be upstreamed eventually, as it is needed to ensure that src attributes on cells don't cause the pass to abort.

proc
equiv_opt -assert -async2sync -map +/gatemate/cells_sim.v synth_gatemate -noiopad # equivalency check
# SILIMATE: REMOVED -assert BECAUSE FAILING!!!
equiv_opt -async2sync -map +/gatemate/cells_sim.v synth_gatemate -noiopad # equivalency check
Copy link
Member

Choose a reason for hiding this comment

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

This is not related to the PR as written, but I have the same response that the -assert is a part of the test and should be kept.

@KrystalDelusion
Copy link
Member

@akashlevy Do you know why the -assert flag was removed on these tests? My inclination is that if it was failing on the Silimate fork it's probably indicative of a problem and shouldn't have been ignored. Without the -assert there's no reason for it to be running equiv_opt at all and it could've just been a techmap.

@akashlevy
Copy link
Contributor

These assertion failures are expected because we changed some stuff in techmap. I didn't intend for any of this to get upstreamed. @nataliakokoromyti can you please create issues in https://github.com/Silimate/bounties with your branches, so I can review what you are doing before you send upstream? That way, we can align on what should be upstreamed and save me and @KrystalDelusion some time

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