Placer places posedge and negedge flip flops into the same slice

[jschj]

The root problem seems to be the following: The constraints regarding clock polarity are checked in here. It uses ArchCellInfo::ffInfo->is_clkinv. ArchCellInfo::ffInfo->is_clkinv is set here. However, ArchCellInfo::ffInfo->is_clkinv is false even if the underlying FF type is FDRE_1 (inverted). This is the case if CellInfo::params contains that string. This is the way how the post place validity is checked here. That is inconsistent, right?

I don't know how to fix this properly as I know nothing about the inner workings.

A quick hack for now might be inserting the following into arch_place.cc at the location mentioned before.

std::string orig_type_name = str_or_default(cell->attrs, id_X_ORIG_TYPE, "");
if (orig_type_name == "FDRE_1")
    cell->ffInfo.is_clkinv = true;

[hansfbaier]

Many thanks for the detailed report and the fix ideas. I hope I will be able to look into this by the end of the week. If not, please remind me. Do you have a simple test design to reproduce the issue?

[jschj]

yosys version: Yosys 0.37+15 (git sha1 6282c1f27, clang 16.0.6 -fPIC -Os) nextpnr-xilinx version: "nextpnr-xilinx" -- Next Generation Place and Route (Version 0.6.0-8-gcff70d67)

All required files should be in the zip file. The chipdb file is quite large.

issue.zip

[hansfbaier]

A quick hack for now might be inserting the following into arch_place.cc at the location mentioned before.

std::string orig_type_name = str_or_default(cell->attrs, id_X_ORIG_TYPE, "");
if (orig_type_name == "FDRE_1")
    cell->ffInfo.is_clkinv = true;

Which code location are you referring to here, exactly?

I think you are right that checking for inverted clocks for the _1 flip flop primitives in fasm.cc is too late. (I added that there when adding negedge FF support). I could not find any other code location, where the IS_CLK_INVERTED is set, so it could only be set by user annotations, but that makes no sense for a user. So i think you are right that somewhere in the packer (not the placer), when the FFs are configured, that the _1 FF types need to set IS_CLK_INVERTEED or ffInfo.is_clkinv.

Unfortunately I also don't understand the inner workings totally, so I have no real advantage over you here. :] So I very much appreciate your effort of looking into this and are willing to assist as much as I can. We can setup a video call if you like and look into this together.

[hansfbaier]

As the packer is the right place to set IS_CLK_INVERTED, I think this would be the right location to set it:

[hansfbaier]

Ah, I think I found the bug: Here I set the wrong attribute (typo). That should be IS_CLK_INVERTED.

[hansfbaier]

I pushed a fix: Please try it and let me know how it works: https://github.com/openXC7/nextpnr-xilinx/tree/stable-backports

[jschj]

I cherry picked the commit onto the main branch and it seems to work. stable-backports does not compile out of the box for me because I don't have boost python for example. I just wondered where is difference between main and stable-backports? And thanks for the fix of course! :)

[hansfbaier]

Thanks for testing! Glad to hear it worked! The main branch contains the backport of upstream changes to the framework. Unfortunately that broke router2 somehow, so that it goes into endless loops sometimes. All binary releases are based on stable-backports. main is not recommended for productive use. That doesn't mean that it would not work for you in some cases. Do you want to make a PR for the main branch?

[jschj]

Do you want to make a PR for the main branch?

I think it's fine for now. But it's good to know where to look in case I encounter problems in the future.

[hansfbaier]

OK, I pushed the changes to the main branch. Please test again to verify it still works.

[hansfbaier]

@jschj ^

[jschj]

It still works :)

[hansfbaier]

Great, thanks!

[hansfbaier]

I also updated the nix toolchain to include the fix.