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

Just crashes on ICESTORM_LC instantiation. #365

Open
BigET opened this issue Dec 2, 2019 · 11 comments
Open

Just crashes on ICESTORM_LC instantiation. #365

BigET opened this issue Dec 2, 2019 · 11 comments

Comments

@BigET
Copy link

BigET commented Dec 2, 2019

Version:

nextpnr-ice40 -- Next Generation Place and Route (git sha1 2898d81)

Input:

// look in pins.pcf for all the pin names on the TinyFPGA BX board
module top (
    input CLK,    // 16MHz clock
    output PIN_15,   // User/boot LED next to power LED
    input PIN_1
);
    ICESTORM_LC #(.LUT_INIT (16'hf505), .CARRY_ENABLE(1), .DFF_ENABLE(1), .CIN_CONST(1), .CIN_SET(1))
        siftRegisterQ(.I1(PIN_1), .LO(PIN_15), .CLK(CLK));

endmodule

Result

terminate called after throwing an instance of 'std::out_of_range'

what(): _Map_base::at

@daveshah1
Copy link
Contributor

Right now ICESTORM_LC is really an internal primitive not intended for end user use. So I will tag #364 and #365 as enhancements rather than bugs for now but hope to deal with them in the next few weeks.

@BigET
Copy link
Author

BigET commented Dec 3, 2019

Is there a documentation about what to use when I want to get a specific outcome?

Using SB_LUT4, SB_CARRY and SB_DFF will not help since the nextpnr will not fuse them (well, I saw that the LUT and DFF are getting fused, but not the CARRY).

@smunaut
Copy link
Contributor

smunaut commented Dec 3, 2019

Are you sure what you're asking is valid ?

@BigET
Copy link
Author

BigET commented Dec 3, 2019

In my mind it is clear, like the __asm directive in C.

I read the specs (ICE40 datasheet/iceStorm) and I want a special logic tile, or a stack of LCs.

It is the way to develop building blocks that then can be frozen and reused, multiplied. It might not be better then what an optimizer for the whole FPGA can master, but it is better in the way that you can incrementally build your FPGA.

This is also the way to do partial modification of an FPGA on the fly. Think that a number of tiles are frozen and let the compiler manage with the rest.

It might be as portable as the assembler code is (ok between x86s, but cannot go to riscV). But that is ok from my point of view.

@smunaut
Copy link
Contributor

smunaut commented Dec 3, 2019

I'm talking about the SB_LUT4 SB_CARRY SB_DFF connections you're asking ... if they're physically doable inside the fpga which might explain why nextpnr isn't packing them in the same LC.

Also you can lock theses down to specific locations if you want ...

@BigET
Copy link
Author

BigET commented Dec 3, 2019

Well, I saw that yosys can take a verilog like x<=x+1; and yields a bunch of SB_LUT4 SB_CARRY SB_DFF, and the nextpnr somehow knows to combine them into a stack of LCs.

However if I do the same, use SB_* in my verilog, nextpnr will not bind them together.

You can see an example in:

https://stackoverflow.com/questions/59154892/how-to-write-the-verilog-to-force-yosys-nextpnr-to-output-a-manually-designed

So if ICESTORM_LC is an internal thing, then what are the hints that I can give to nextpnr to instruct the fusing of a SB_* triplet.

@smunaut
Copy link
Contributor

smunaut commented Dec 3, 2019

Yes and looking at that code, it's not implementable.

The "Carry In" to a LC replaces the I3 of the LUT, they need to be connected to the same wire, it's a physical limitation of the FPGA.

So to be in the same LC, I3 of the LUT must be connected to the CI of the SB_CARRY.

Also:

  • I1 of SB_LUT4 must be connector to I2 of SB_CARRY in the same LC
  • I2 of SB_LUT4 must be connected to I1 of SB_CARRY in the same LC
  • I3 of SB_LUT4 must be connector to CO of SB_CARRY of the LC below.

Those are limitation of the iCE40 architecture. If you obey them, then nextpnr will pack them properly.

@BigET
Copy link
Author

BigET commented Dec 3, 2019

Maybe we should continue this on the stack overflow.

But.

It is my understanding that when I use the I3 input as an input, the "carry in" will still go to the carry unit regardless.

The mux between the "carry in" and the I3 input is affecting only the LUT and not the carry.

Here is the carry function from icestorm project :

lutff_i/cout = lutff_i/in_1 + lutff_i/in_2 + lutff_(i-1)/cout > 1

@newhouseb
Copy link

I'm trying to pack things tightly (for a Time-to-Digital Converter) and ran into this issue as well. I rebuilt nextpnr with debug symbols and caught the exception for the following code:

    (* keep *) ICESTORM_LC #(.LUT_INIT(16'hcaca),
                 .CARRY_ENABLE(1),
                 .DFF_ENABLE(1))
        MANUAL_LUT ( 
                .I0(1'b0),
                .I1(1'b0),
                .I2(1'b0),
                .I3(1'b1),
                .CIN(1'b0),
                .CLK(clk)
        );
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff461c801 in __GI_abort () at abort.c:79
#2  0x00007ffff500f957 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ffff5015ab6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff5015af1 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff5015d24 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x00007ffff501182f in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#7  0x00005555558c0c3d in std::__detail::_Map_base<nextpnr_ice40::IdString, std::pair<nextpnr_ice40::IdString const, nextpnr_ice40::PortInfo>, std::allocator<std::pair<nextpnr_ice40::IdString const, nextpnr_ice40::PortInfo> >, std::__detail::_Select1st, std::equal_to<nextpnr_ice40::IdString>, std::hash<nextpnr_ice40::IdString>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true>, true>::at (__k=..., this=<optimized out>) at /usr/include/c++/7/bits/hashtable_policy.h:793
#8  std::unordered_map<nextpnr_ice40::IdString, nextpnr_ice40::PortInfo, std::hash<nextpnr_ice40::IdString>, std::equal_to<nextpnr_ice40::IdString>, std::allocator<std::pair<nextpnr_ice40::IdString const, nextpnr_ice40::PortInfo> > >::at (__k=..., this=<optimized out>) at /usr/include/c++/7/bits/unordered_map.h:994
#9  nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#3}::operator()(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*) const (cell=<optimized out>, ctx=0x555563a771b0, __closure=<synthetic pointer>) at /home/ben/src/nextpnr/ice40/chains.cc:247
#10 nextpnr_ice40::find_chains<nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#1}, nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#2}, nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#3}>(nextpnr_ice40::Context const*, nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#1}, nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#2}, nextpnr_ice40::ChainConstrainer::process_carries()::{lambda(nextpnr_ice40::Context const*, nextpnr_ice40::CellInfo const*)#3}, unsigned long) (ctx=0x555563a771b0, cell_type_predicate=..., get_previous=..., get_next=..., min_length=min_length@entry=2) at /home/ben/src/nextpnr/common/chain_utils.h:56
#11 0x00005555558c3cac in nextpnr_ice40::ChainConstrainer::process_carries (this=this@entry=0x7fffffffcfb0) at /home/ben/src/nextpnr/ice40/chains.cc:255
#12 0x00005555558bf7a8 in nextpnr_ice40::ChainConstrainer::constrain_chains (this=0x7fffffffcfb0) at /home/ben/src/nextpnr/ice40/chains.cc:309
#13 nextpnr_ice40::constrain_chains (ctx=ctx@entry=0x555563a771b0) at /home/ben/src/nextpnr/ice40/chains.cc:315
#14 0x00005555558de860 in nextpnr_ice40::Arch::pack (this=0x555563a771b0) at /home/ben/src/nextpnr/ice40/pack.cc:1489
#15 0x00005555557bc858 in nextpnr_ice40::CommandHandler::executeMain (this=this@entry=0x7fffffffda20, ctx=std::unique_ptr<nextpnr_ice40::Context> = {...}) at /home/ben/src/nextpnr/common/command.cc:310
#16 0x00005555557bce76 in nextpnr_ice40::CommandHandler::exec (this=0x7fffffffda20) at /home/ben/src/nextpnr/common/command.cc:387
#17 0x00005555557b0724 in main (argc=<optimized out>, argv=<optimized out>) at /home/ben/src/nextpnr/ice40/main.cc:232
(gdb) f 14
#14 0x00005555558de860 in nextpnr_ice40::Arch::pack (this=0x555563a771b0) at /home/ben/src/nextpnr/ice40/pack.cc:1489
1489	        constrain_chains(ctx);
(gdb) 

Which corresponds to this line:

net_only_drives(ctx, cell->ports.at(ctx->id("COUT")).net, is_lc, ctx->id("CIN"), false);

Which seems to expect that .COUT is defined (perhaps yosys normally defines every port?). So if I do the following, the error goes away and things route properly.

    wire dummy;

    (* keep *) ICESTORM_LC #(.LUT_INIT(16'hcaca),
                 .CARRY_ENABLE(1),
                 .DFF_ENABLE(1))
        MANUAL_LUT ( 
                .I0(1'b0),
                .I1(1'b0),
                .I2(1'b0),
                .I3(1'b1),
                .CIN(1'b0),
                .COUT(dummy),
                .CLK(clk)
        );

    assign LED_R = dummy;

@newhouseb
Copy link

Alright, I've been starting at nextpnr code for a number of hours now had have some semblance of what's going on (and why I'm struggling to manually place an ICESTORM_LC):

One of the steps nextpnr does is to identify "carry chains" that need to be placed (more or less) vertically. If a carry is enabled for a cell, it is coalesced into a chain with additional ICESTORM_LCs at the top and bottom. Only the top of the chain is actually placed via the specified algorithm (HEaP by default), everything else is simply defined relative to the top of the chain (I think there are some nuances though). The extra top (and bottom) ICESTORM_LCs don't inherit any BEL placement from any of the child cells thus it's never used.

I've tried a couple different modifications to the code to no-avail:

  • Tried to exclude my MANUAL_LUT from being coalesced into a Carry Chain. This explodes.
  • Tried to transfer BEL placement to the top of the Carry Chain (DFF & LUT merging/packing does this). This explodes as well.

@newhouseb
Copy link

Finally getting somewhere!

This commit: newhouseb@39f15c4 enables me to sort of manually place a cell, using the following code (note the BEL attribute):

       wire top_carry_in;
       assign top_carry_in = din0;
       wire top_c1 = 1;
       wire top_c2 = 0;
       wire top_lut_out;
       wire top_carry_out;
       wire top_dff_out;
       (* keep *) SB_CARRY AAA_TOP_CARRY (.CO(top_carry_out), .CI(top_carry_in), .I0(top_c1), .I1(top_c2));
       (* keep *) SB_LUT4 #(.LUT_INIT(16'd1)) AAA_TOP_LUT (
                .I0(1'b0),
                .I1(top_c1),
                .I2(top_c2),
                .I3(top_carry_in),
                .O(top_lut_out)
        );
        (*  BEL="X9/Y1/lc0", keep *) SB_DFF AAA_TOP_DFF (top_dff_out, clk, top_lut_out);

       wire middle_carry_in;
       assign middle_carry_in = top_carry_out;
       wire middle_c1 = 1;
       wire middle_c2 = 0;
       wire middle_lut_out;
       wire middle_carry_out;
       wire middle_dff_out;
       (* keep *) SB_CARRY AAA_MIDDLE_CARRY (.CO(middle_carry_out), .CI(middle_carry_in), .I0(middle_c1), .I1(middle_c2));
       (* keep *) SB_LUT4 #(.LUT_INIT(16'd1)) AAA_MIDDLE_LUT (
                .I0(1'b0),
                .I1(middle_c1),
                .I2(middle_c2),
                .I3(middle_carry_in),
                .O(middle_lut_out)
        );
        (* keep *) SB_DFF AAA_MIDDLE_DFF (middle_dff_out, clk, middle_lut_out);

       wire bottom_carry_in;
       assign bottom_carry_in = middle_carry_out;
       wire bottom_c1 = 1;
       wire bottom_c2 = 0;
       wire bottom_lut_out;
       wire bottom_carry_out;
       wire bottom_dff_out;
       (* keep *) SB_CARRY AAA_BOTTOM_CARRY (.CO(bottom_carry_out), .CI(bottom_carry_in), .I0(bottom_c1), .I1(bottom_c2));
       (* keep *) SB_LUT4 #(.LUT_INIT(16'd1)) AAA_BOTTOM_LUT (
                .I0(1'b0),
                .I1(bottom_c1),
                .I2(bottom_c2),
                .I3(bottom_carry_in),
                .O(bottom_lut_out)
        );
        (* keep *) SB_DFF AAA_BOTTOM_DFF (bottom_dff_out, clk, bottom_lut_out);

It does so through a couple modifications:

  • When a cell is wrapped up in a carry chain any BEL attributes from the first cell in the chain is transferred to the parent cell.
  • When placing is run any cells with placement constraints derive placement constraints for any children in their chain.

Some funky things about this:

  • Specifying a location for a logic cell, actually ends up specifying its parent (assuming it's at the front of a carry chain).
  • Ideally we shouldn't need to infer a dummy logic cell at the beginning and end of a carry chain, but perhaps this is a misunderstanding on my part.
  • I have no idea what happens when carry chains span multiple 8-cell blocks.

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

No branches or pull requests

4 participants