Add PolarFire FPGA support #4474

tony-min-1 · 2024-07-03T14:04:12Z

What are the reasons/motivation for this change?

Adding new support for PolarFire FPGA as synth_mchp pass

Explain how this is achieved.

The overall flow is very similar to synth_xilinx.
Added DSP and RAM inference based on PolarFire macro library guide and user fabric guide
- DSP inference achieved using pmgen, RAM inference uses memory_libmap pass
Rest of simulation model taken from existing SmartFusion2 support because it has similar architecture to PolarFire

If applicable, please suggest to reviewers how they can test the change.

Added some basic verilog tests. Ran equivalence check on the synthesized output Verilog

Credits to Damon Ma for adding the Verilog tests + code cleanups

Ravenslofty

So, there's a lot of code here. Here are some general thoughts before going over this with a fine-toothed comb.

Yosys always spells out the name of vendors and families; synth_lattice, synth_ice40. so, the mchp and pf abbreviation should be microchip and polarfire.
architecture-specific tests go in tests/arch/XXX, rather than techlibs/XXX/tests. I suggest referencing the generic testbench files in tests/arch/common, and also looking at how synthesis testbenches are idiomatically formed.

techlibs/mchp/cells_sim.v

…e_filenames

changes made to filenames + references

…ochip

Move tests

add missing INIT for uSRAM

tony-min-1 · 2024-07-04T20:44:52Z

So, there's a lot of code here. Here are some general thoughts before going over this with a fine-toothed comb.

Yosys always spells out the name of vendors and families; synth_lattice, synth_ice40. so, the mchp and pf abbreviation should be microchip and polarfire.

architecture-specific tests go in tests/arch/XXX, rather than techlibs/XXX/tests. I suggest referencing the generic testbench files in tests/arch/common, and also looking at how synthesis testbenches are idiomatically formed.

changed all references of mchp to microchip and pf to polarfire
Moved all tests into tests/arch/microchip and setup the tests in same way as other architecture specific tests
- enabled microchip tests in Makefile

Ravenslofty

I think all the DSP testcases should be merged together to avoid too much litter.

But here's a first pass review.

Ravenslofty · 2024-07-04T21:10:42Z

techlibs/microchip/cells_sim.v

+	endspecify
+endmodule
+
+(* abc9_lut=2 *)


This doesn't look right to me, though I'm unfamiliar with the PolarFire. If you have a LUT4 architecture I would expect everything to have an area of 1, which implicitly tries to make ABC9 use LUT4s for as much logic as possible. Does it really require twice as much area for a LUT2 than a LUT1, and three times as much area for a LUT3?

yes, it should just be an area of 1 for all LUTs in this architecture.

Ravenslofty · 2024-07-04T21:11:55Z

techlibs/microchip/cells_sim.v

+	input A, B, C, D,
+	output Y
+);
+	assign Y = A & B & C & D;


Is there dedicated hardware for this, or are you expecting this to map to soft logic?

This is from the macro library, I would expect this to map to a LUT4.

but as implemented, it won't; you will need to techmap it into soft logic, or alternatively directly into a LUT.

Libero actually recognizes these macros. You can manually instantiate these cells.

techlibs/microchip/cells_sim.v

Ravenslofty · 2024-07-04T21:22:42Z

techlibs/microchip/microchip_dffopt.cc

+					if (it_CE != bit_to_lut.end())
+						worthy_post_ce = true;
+				} else if (sig_CE.data != State::S1) {
+					// Strange.  Should not happen in a reasonable flow, so bail.


Maybe a log_assert on this to fail noisily if this occurs?

Ravenslofty · 2024-07-04T21:24:50Z

techlibs/microchip/synth_microchip.cc

+		log("    synth_microchip [options]\n");
+		log("\n");
+		log("This command runs synthesis for Microchip FPGAs. Operating on\n");
+		log("partly selected designs is not supported (you must submit a fully-selected \n");


Generally we imply this and check it with an assert; it makes the help a little easier to understand.

Will remove this line. There is already a check for fully selected design + error msg if design is not fully selected

Ravenslofty · 2024-07-04T21:44:28Z

tests/arch/microchip/large_mult.v

+OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+
+module large_mult(


Use tests/arch/common/mul.v

Ravenslofty · 2024-07-04T21:47:58Z

tests/arch/microchip/ram_SDP.v

+OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+
+module ram_SDP(data,waddr,we,clk,q);


The test semantics aren't exactly the same, but this looks like sync_ram_sdp from tests/arch/common/blockram.v

Ravenslofty · 2024-07-04T21:49:04Z

tests/arch/microchip/ram_TDP.v

+OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+
+module ram_TDP (clka,clkb,wea,addra,dataina,qa,web,addrb,datainb,qb);


sync_ram_tdp from tests/arch/common/blockram.v

Ravenslofty · 2024-07-04T21:53:24Z

tests/arch/microchip/simple_ram.v

+assign dout = mem[addr_reg];
+
+always@(posedge clk) begin
+	addr_reg <= addr;


That's...an odd way to write this. I'm a little confused about the intended semantics here.

it is intended to be functionally same as sync_ram_sp from tests/arch/common/blockram.v . I've changed to use sync_ram_sp instead.

Ravenslofty · 2024-07-04T21:57:33Z

tests/arch/microchip/widemux.v

+OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+
+module widemux(


mux8 from tests/arch/common/mux.v

mux4 generated using case is a little different. Having a bit of trouble inferring MX4 macro

what's the trouble, may I ask?

So I'm interested in $_MUX4_ cells. To discover this cell structure, I use muxcover to pack $_MUX_ trees into $_MUX4_. With the case statement, you get $pmux instead of a tree of $_MUX_. I couldn't find a way to cleanly convert $pmux into $mux tree. I have tried using pmuxtree and techmap -map +/pmux2mux.v but there is always additional logic (i.e. $eq) driving the select control of $mux, which I assume makes it difficult for muxcover to pack into $_MUX4_

Then the answer is that you're calling muxcover too early. The reason you're not getting any $_MUX4_ cells is because you have no $_MUX_ cells which you get from techmapping $mux cells.

before muxcover, I call simplemap t:$mux to do the conversion from $mux to $_MUX_ cells. And before simplemap t:$mux is where I experimented with the $pmux -> $mux conversions.

* area should be 1 for all LUTs * clean up macros * add log_assert to fail noisily when encountering oddly configured DFF * clean help msg * flatten set to true by default * update * merge mult tests * remove redundant test * move all dsp tests to single file and remove redundant tests * update ram tests * add more dff tests * fix c++20 compile errors * add option to dump verilog * default to use abc9 * remove -abc9 option since its the default now --------- Co-authored-by: tony <minchunlin@gmail.com>

tony-min-1 · 2024-07-08T14:59:40Z

I think all the DSP testcases should be merged together to avoid too much litter.

But here's a first pass review.

Thanks for the feedbacks, I have made the following changes:

Organize similar tests together in single file to reduce clutter
Re-use existing tests from tests/arch/common
Fix c++20 compile error
use abc9 by default
flatten hierarchy by default
add vlog option to dump Verilog
various cleanups

Ravenslofty

last few things I see. maybe the arch-specific tests could be inlined into the yosys scripts that use them; that would cut down the PR line count a bit more.

Ravenslofty · 2024-07-08T17:34:07Z

techlibs/microchip/synth_microchip.cc

+		log("        Run 'abc' with '-D 1' option to enable flip-flop retiming.\n");
+		log("        implies -dff.\n");
+		log("\n");
+		log("    -abc\n");


this is conventionally -noabc9.

Ravenslofty · 2024-07-08T17:39:23Z

techlibs/microchip/synth_microchip.cc

+			else if (abc9) {
+
+				std::string abc9_opts;
+				// for the if command in abc to specify wire delay between adjects LUTs (default = 0)


Suggested change

// for the if command in abc to specify wire delay between adjects LUTs (default = 0)

// for the if command in abc to specify wire delay between adjacent LUTs (default = 0)

Ravenslofty · 2024-07-08T17:45:38Z

tests/arch/microchip/widemux.v

+OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+
+module widemux(


what's the trouble, may I ask?

…do not support init

tony-min-1 · 2024-07-08T21:08:03Z

Inlined all tests into .ys script.
Realized that PolarFire DFF does not support init. Added switch to remove init attribute, this was done the same way as in SmartFusion2 pass.

tony-min-1 · 2024-07-17T14:28:47Z

Is there any other changes we need to make?

tony-min-1 added 6 commits July 2, 2024 12:44

add PolarFire FPGA support

acddc36

replace space indent with tab indent

2ced275

more indent fix

68a11c9

fix indent

f57b624

remove DSP48 references

e5bdc9b

fix cells_sim.v

7770fa7

Ravenslofty requested changes Jul 4, 2024

View reviewed changes

techlibs/mchp/cells_sim.v Outdated Show resolved Hide resolved

Ravenslofty reviewed Jul 4, 2024

View reviewed changes

techlibs/mchp/cells_sim.v Outdated Show resolved Hide resolved

Damon-Ma-MCHP and others added 13 commits July 4, 2024 08:53

changes made to filenames + references

0bb7d13

Merge branch 'mchp' of https://github.com/tony-min-1/yosys into chang…

59e45be

…e_filenames

fixed typos, build with makefile succeeds

d0cd01a

rename options

c385421

missed a few pf instances

6b80e02

another typo

5ba06fd

Merge pull request #1 from tony-min-1/change_filenames

7ff8912

changes made to filenames + references

use output reg instead of additional reg declaration

19d3214

move microchip tests from techlibs/microchip/tests to tests/arch/micr…

e3c4791

…ochip

add assertions for synth_microchip tests

8e7ec2d

ininclude microchip tests in makefile

9de5602

Merge pull request #2 from tony-min-1/move_tests

e9ff5f7

Move tests

Add missing u sram init (#3)

6fe0e00

add missing INIT for uSRAM

tony-min-1 requested a review from Ravenslofty July 4, 2024 20:52

Ravenslofty requested changes Jul 4, 2024

View reviewed changes

tony-min-1 requested a review from Ravenslofty July 8, 2024 15:05

tony-min-1 added 3 commits July 8, 2024 11:13

fix compile warning

3c95a28

undo last change, to investigate dff_opt test failure

a0c9d10

initialize argidx to 1

af67c74

Ravenslofty requested changes Jul 8, 2024

View reviewed changes

tony-min-1 added 2 commits July 8, 2024 15:10

cosmetic changes

0afb5e2

inline all tests. Add switch to remove init values as PolarFire DFFs …

3db69b7

…do not support init

tony-min-1 requested a review from Ravenslofty July 8, 2024 21:05

Ravenslofty approved these changes Jul 18, 2024

View reviewed changes

Ravenslofty added the merge-after-jf Merge: PR will be merged after the next Dev JF unless concerns are raised label Jul 18, 2024

nakengelhardt merged commit 9f869b2 into YosysHQ:main Jul 29, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PolarFire FPGA support #4474

Add PolarFire FPGA support #4474

tony-min-1 commented Jul 3, 2024

Ravenslofty left a comment

tony-min-1 commented Jul 4, 2024

Ravenslofty left a comment

Ravenslofty Jul 4, 2024

tony-min-1 Jul 5, 2024

Ravenslofty Jul 4, 2024

tony-min-1 Jul 5, 2024

Ravenslofty Jul 5, 2024

tony-min-1 Jul 8, 2024

Ravenslofty Jul 4, 2024

Ravenslofty Jul 4, 2024

tony-min-1 Jul 5, 2024 •

edited

Loading

Ravenslofty Jul 4, 2024

Ravenslofty Jul 4, 2024

Ravenslofty Jul 4, 2024

Ravenslofty Jul 4, 2024

tony-min-1 Jul 5, 2024 •

edited

Loading

Ravenslofty Jul 4, 2024

tony-min-1 Jul 5, 2024

Ravenslofty Jul 8, 2024

tony-min-1 Jul 8, 2024

Ravenslofty Jul 8, 2024

tony-min-1 Jul 8, 2024

tony-min-1 commented Jul 8, 2024

Ravenslofty left a comment

Ravenslofty Jul 8, 2024

Ravenslofty Jul 8, 2024

Ravenslofty Jul 8, 2024

tony-min-1 commented Jul 8, 2024

tony-min-1 commented Jul 17, 2024

	// for the if command in abc to specify wire delay between adjects LUTs (default = 0)
	// for the if command in abc to specify wire delay between adjacent LUTs (default = 0)

Add PolarFire FPGA support #4474

Add PolarFire FPGA support #4474

Conversation

tony-min-1 commented Jul 3, 2024

Ravenslofty left a comment

Choose a reason for hiding this comment

tony-min-1 commented Jul 4, 2024

Ravenslofty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-min-1 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-min-1 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-min-1 commented Jul 8, 2024

Ravenslofty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-min-1 commented Jul 8, 2024

tony-min-1 commented Jul 17, 2024

tony-min-1 Jul 5, 2024 •

edited

Loading

tony-min-1 Jul 5, 2024 •

edited

Loading