Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Libero Updates #398

Merged
merged 4 commits into from
Oct 13, 2023
Merged

Libero Updates #398

merged 4 commits into from
Oct 13, 2023

Conversation

paul-gatewood
Copy link
Contributor

Adds support for Libero netlist constraints and for --no-export with Libero. Also fixes some issues I was having with the PR checks.

@olofk
Copy link
Owner

olofk commented Oct 10, 2023

Thank you for this. I have some questions about the changes and the libero backend in general. I did not write that one myself, and looking at it now I'm a bit confused about certain things.

But I will start right away with pulling in your two first commits as I'm seeing the CI failures as well, that I believe these are aiming to fix.

@olofk
Copy link
Owner

olofk commented Oct 10, 2023

First two patches to fix CI are puled in now. Thanks for those!

So, the first thing I wonder now is if there is a compelling reason to ever use import_files instead of create_links? I understand that `import_files' is what has been used up until now but I'm not convinced it was done for the right reasons. Then again, I'm not a Libero user myself, so I can't know for sure. But looking at the code I believe this could simplify things a bit. What do you think?

Paul Gatewood added 3 commits October 11, 2023 09:55
Libero has two types of netlist constraints files that are synthesis
only, so they didn't work with the existing setup. Adding specific
filters for synthesis and place/route simplified the changes needed to
the template.
Since Libero has a separate command for exporting files versus linking
files, I added an export_files parameter to edatool to pass to the
templates. I also made the global include paths relative to make it
easier to move the project directory.
Always use `create_links` instead of `import_files` in the Libero
project script to simplify the --no-export implementation.
@paul-gatewood
Copy link
Contributor Author

paul-gatewood commented Oct 11, 2023

Of course, I'm glad I can contribute! That's a great point, I can't actually think of a use case for import_files that wouldn't be covered by edalize's manual export to the src directory (or maybe that's handled by fusesoc). I ran through some tests on a project of mine, and it looks like just using create_links works great with or without --no-export.

@@ -186,19 +194,39 @@ def tcl_file_filter(self, f):
return file_types[_file_type] + f.name
return ""

def constraint_file_filter(self, f, type="ALL"):
def syn_constraint_file_filter(self, f):
file_types = {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really understand what this dict does. Could you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that dict used to supply the path to the constraints files when they were imported, but it's obsolete now that we're always linking. I'll take a stab at cleaning that up.

"FDC": "constraint/",
"NDC": "constraint/",
}
_file_type = f.file_type.split("-")[0]
Copy link
Owner

Choose a reason for hiding this comment

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

What is this split for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell, it might just be used to convert file_types like vhdlSource-2008 to just vhdlSource. It seems like that pattern's used in a few other toolchains, like quartus.

Copy link
Owner

Choose a reason for hiding this comment

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

That would be my guess too, but since none of the potential matches contain dashes in the file type I suspect this whole function (as well as the other ones) can just be simplified to:

    def pnr_constraint_file_filter(self, f):
        if _file_type in ["PDC", "SDC", "FPPDC"]:
            return f.name

return f.name
return ""

def constraint_file_filter(self, f, type="ALL"):
Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need this function now that we have the syn and pnr versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was still used in one place for the timing constraints, but I replaced that with a simpler filter and removed this one.

@olofk
Copy link
Owner

olofk commented Oct 12, 2023

Thanks. Happy to see the simplifications after dropping the export parts (and to answer your other question, it's technically FuseSoC that does the exporting). In general it looks good, but I made some comments where I think we can actually make further simplifcations.

Since we're no longer importing the constraints files, we don't have to
worry about converting the file names to point to the import paths, so
the filters can be much simpler.
@olofk olofk merged commit df39371 into olofk:main Oct 13, 2023
7 checks passed
@olofk
Copy link
Owner

olofk commented Oct 13, 2023

Many thanks for your contributions. Squashed and merged!

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.

None yet

2 participants