Potential compiler bug when indexing register

[laanwj]

Not sure where to report this, but I think I found a compiler bug somewhere in the yosys+arachne-pnr+icestorm toolchain, and I don't know how to go about debugging it.

These two verilog sources, which run a simple program from ROM when 'a' is sent to the UART, differing in one line, should be equivalent in their result:

https://github.com/laanwj/yosys-ice-experiments/blob/master/error/ok.v#L126 https://github.com/laanwj/yosys-ice-experiments/blob/master/error/fail.v#L123

The first prints a colorful 'hello', however the second gets ptr wrong and prints random garbage.

The ROM 'program' is assembled here, with the problematic instruction highlighted: https://github.com/laanwj/yosys-ice-experiments/blob/master/error/build.py#L47

The opcode is 0xC8, which means both rom_rdb[1] and rom_rdb[0] are 1'b0 (=register r0). So as I see it {rom_rdb[1],1'b0} and .rom_rdb[1:0] should result in 2b'00. The first however (in fail.v) produces a result I cannot explain.

Apologies in advance if this is just a misunderstanding in how verilog is supposed to work ... (but that an alternative formulation like cpuregs[rom_rdb&2'h2] has the same problem convinced me that something may be wrong)

[cliffordwolf]

[...] and I don't know how to go about debugging it.

You should always simulate your circuits. I have made the necessary changes to your test case (yosys is a bit relaxed with what verilog code it accepts compared to what simulators like Icarus Verilog usually expect) and added a small test bench:

• http://scratch.clifford.at/laanwj-yosys-ice-experiments-tb.patch

Simulating with this testbench reveals that the circuit does not meet its spec either way:

• http://i.imgur.com/E0hNz3I.png

Because the uart output port valid, stays high, the code

if (uart0_valid && uart0_data_in == "a") begin
    ptr <= 9'h0;
    state <= S_OP;
end

is always active and the FSM is stuck at ptr=0 and state=S_OP.

Please use this patch to build a proper testbench that can be used to validate the circuit behavior in a simulation environment. With a working testbench I can run post-synthesis simulations that allow me to isolate the cause of the diverging post-synthesis circuit behavior.

[laanwj]

Thanks for the patch, I'll try getting the testbench to work first. Er yes - the UART read should definitely only happen in S_IDLE, and only for one character.

[laanwj]

Ok, updated the repository with the testbench. Both ok and fail produce the same output, and it looks like the correct one (shortened the test pattern to just 'hello\n' or

00010110 10100110 00110110 00110110 11110110 01010000

Waveform files are byte-wise equal.

[cliffordwolf]

Just a short update:

• I have now reproduced the problem in hardware and will further investigate later today.
• Is it possible that you forgot to git add testbench.sh and testbench.v?

[laanwj]

Oops - pushed the testbench script and v

[cliffordwolf]

Another update: I have now isolated to problem to be in the resource sharing pass in yosys. I will investigate the issue further over the weekend..

Here is another patch for your experiment, reflecting the current state of my troubleshooting efforts: http://scratch.clifford.at/laanwj-yosys-ice-experiments-upd2.patch

jfyi: The sed -i 's/WCLKN/WCLK/; ... in the Makefile is a temporary workaround. Working with your code, I have now realized that the iCE40 primitive with negative egde clocks have Ns appended to the negative edge clock inputs. This is now corrected in Yosys and icestorm, but it is not yet corrected in arachne-pnr.

[cliffordwolf]

I have now fixed this issue in commit e7c018e5d14c3c609669ab514a7e9111ff006022. Thanks for reporting this problem. Please do not hesitate to report bugs in the future. It is very appreciated.

Just for completeness: Here is a patch with my final version of the test bench (replaces the previous upd2): http://scratch.clifford.at/laanwj-yosys-ice-experiments-upd3.patch

[laanwj]

Thanks a lot for tracking the problem down and fixing!

I'll apply your patch and re-test next week. I've been in travel last few days so haven't been able to do much.

[laanwj]

Confirmed fixed.

I did have a bug in my experiment with regard to initialization. What is the recommended way to initialize values in synthesizable logic with this toolchain on ICE FPGAs?

• Initializer values on registers: this results in "Warning: Wire top._uart0._rx.hh has an unprocessed 'init' attribute." so I suppose not.
• Explicit reset logic
  • Synchronous/asynchronous preferred?
  • Is it possible to trigger this automatically after the device is flashed or powered on? Currently I make use of an assumption that a register starts out at 0, then trigger reset on the first clock pulse, but if initial state is undefined that's fragile
  • Wire one of the serial control pins to the reset (as in swapforth) - this is good for resetting it later, but doesn't seem to be required initially
• initial blocks: haven't tried this, I suppose not

I haven't been able to find anything conclusive as it appears to differ per hardware.

[cliffordwolf]

Initializers such as reg foo = 0; and initial blocks both work with yosys (that's why you get the warning in the first place), but the ice40 architecture does not support initialization values for the registers, so whatever initialization value you set is being ignored. But all registers in the iCE40 are set to 0 when the bitstream is fed into the device, so you can ignore the warning for everything that you want to initialize to zero anyways.

You can use explicit reset logic: The tools and the architecture both support synchronous and asynchronous reset. But as a general rule for HDL design I would recommend synchronous reset over asynchronous reset for various reasons.

Yes, you can also use the fact that registers are initialized to zero to generate a reset pulse. I usually do something like the following (only 5 LCs big and generates a 16 cycles long reset pulse):

reg [3:0] reset_cnt = 0;
wire resetn = &reset_cnt;
always @(posedge clk)
  if (!resetn) reset_cnt <= reset_cnt + 1;

[laanwj]

Thanks - so every register does get initialized to 0 - OK I can't explain the issue I had, then.

I thought it had to do with the state being undefined at startup causing the state machine to not work, so I added the state initialization to the reset block (which indeed uses a one-time pulse by a register initialized to 0) and then it worked.

But given that, it shouldn't have an effect, as 0 is the valid reset state.

I'll try if I can reproduce the issue.

[cliffordwolf]

Thanks - so every register does get initialized to 0 - OK I can't explain the issue I had, then.

I have not found anything in the Lattice documentation that indicates that registers are initialized to 0, but I have done a few of tests to verify this and in my tests it was pretty clear that this is what happens.

That being said: In many applications the clock isn't stable when the device is programmed and thus it is possible that the state of the circuit gets corrupted shortly after power on. So you either want to gate the clock and only let it through to your logic once it has been stabilized, or -- especially if you are already using a PLL -- use the LOCK output of the PLL as inverted reset signal for your design.

However, reg foo; and reg foo = 0; do not always yield the same result! If you need the register to be zero initialized, you have to tell the tools! Otherwise the initial value is assumed to be undefined, which allows yosys to perform more optimizations. Two examples:

(1) If you are using retiming in your flow (run synth_ice40 with -retime), then yosys will possibly move any register that does not have an init value assigned. For example, when a register is moved to the other side of an inverter, then the effective initialization value of that register changes from 0 to 1.

(2) Consider the following code for a simple reset generator:

reg resetn = 0;
always @(posedge clk)
  resetn <= 1;

This will work. However, it won't work if you remove the initialization value like this:

reg resetn;
always @(posedge clk)
  resetn <= 1;

According to the language definition, the resetn register now starts out as undefined and can only be set to 1. Thus the tool may remove the register altogether and simply replace resetn with a constant 1.

I can only further underline the importance of simulation, as simulation correctly initializes non-initialized registers with the special undefined x state and lets you see the difference between zero-initialized and uninitialized FFs. If you only test in hardware, you can have situations where (maybe depending on unrelated changes in the design) the synthesis tool sometimes takes an optimization that break the zero-initialized FF assumption and sometime it does not. You'll never find a bug like this in a complex design without a simulation that correctly models the x state.

[cseed]

I have not found anything in the Lattice documentation that indicates that registers are initialized to 0

Page 5 of the iCE40 LP/HX Family Data Sheet says: "Each DFF also connects to a global reset signal that is automatically asserted immediately following device configuration."

[laanwj]

@cliffordwolf I can reproduce it.

Commits:

• arachne-pnr a94ed39
• yosys 745d561
• icestorm c6f1e1f

yosys-ice-experiments:

• 6d4585b fail.v fails (no reply at all to serial)
• 9db2625 fail.v works (sends back "hello\n")

The only change is (that makes it work again):

diff --git a/error/fail.v b/error/fail.v
index 046ddc1..acc9396 100644
--- a/error/fail.v
+++ b/error/fail.v
@@ -84,7 +84,7 @@ module top(input clk,
     reg [8:0] ptr_saved;
     reg [7:0] outb;
     reg outf;
-    reg [2:0] state = S_IDLE;
+    reg [2:0] state;
     reg [7:0] opcode;
     reg [7:0] cpuregs [3:0];

@@ -168,12 +168,10 @@ module top(input clk,
                 state <= S_OP;
             end
         endcase
-    end
-
-    // Reset logic
-    always @(posedge clk) begin
+        // Reset logic
         if (!uart0_reset) begin // Reset UART only for one clock
             uart0_reset <= 1;
+            state <= S_IDLE;
         end
     end

It has no difference on the result of the testbench/simulation, but does on the hardware (iCEstick) . Could be the unstable-clock-after-reset issue that you mention, but I'm not sure.

@cseed Great!

[cliffordwolf]

Have you pushed 9db2625 yet? I can't see it in your github repo..

[laanwj]

Done!

[cliffordwolf]

At least here testbench_synth_fail and testbench_post_fail both fail for 6d4585b, so it is reproducible in simulation! Therefore this is not an issue with an unstable clock.. I'll investigate further..

[cliffordwolf]

Ok, the problem here is that the FSM subsystem in Yosys does not know anything about initial values. That is also why you do not get a warning for an ignored init attribute for top.state:

yosys  -q -p "synth_ice40 -top top -blif fail.blif" fail.v uart.v
Warning: Wire top._uart0._rx.hh has an unprocessed 'init' attribute.
Warning: Wire top.uart0_reset has an unprocessed 'init' attribute.

After FSM recoding the init value for the state register is gone and S_IDLE state is something like 6'b000001 and 6'b000000 does not code for any valid state in one-hot encoding. So the FSM never starts doing anything. That's also why the problem goes away if you add explicit initialization for the state register.

E.g. adding the (* fsm_encoding = "none" *) attribute works around the problem:

(* fsm_encoding="none" *) reg [2:0] state = S_IDLE;

Now there is also a warning about an ignored init attribute for top.state:

yosys  -q -p "synth_ice40 -top top -blif fail.blif" fail.v uart.v
Warning: Wire top._uart0._rx.hh has an unprocessed 'init' attribute.
Warning: Wire top.state has an unprocessed 'init' attribute.
Warning: Wire top.uart0_reset has an unprocessed 'init' attribute.

Starting with commit b66bf8bed17473b0b972671fe the fsm_detect pass now ignores possible state registers when they have an "init" attribute assigned. This should fix the issue.

[laanwj]

Post-synthesis simulation - of course. Should have compared those too, will do next time.

Thanks! I slightly suspected the fsm detect pass changing state numbers, but wasn't sure how to check. Sounds like a good workaround.