-
Notifications
You must be signed in to change notification settings - Fork 881
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
Potential compiler bug when indexing register #80
Comments
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: Simulating with this testbench reveals that the circuit does not meet its spec either way: Because the uart output port
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. |
Thanks for the patch, I'll try getting the testbench to work first. |
Just a short update:
|
Oops - pushed the testbench script and v |
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: jfyi: The |
I have now fixed this issue in commit e7c018e. 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 |
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. |
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?
I haven't been able to find anything conclusive as it appears to differ per hardware. |
Initializers such as 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):
|
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. |
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, (1) If you are using retiming in your flow (run (2) Consider the following code for a simple reset generator:
This will work. However, it won't work if you remove the initialization value like this:
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 I can only further underline the importance of simulation, as simulation correctly initializes non-initialized registers with the special undefined |
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." |
@cliffordwolf I can reproduce it. Commits:
yosys-ice-experiments:
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) . @cseed Great! |
Have you pushed 9db2625 yet? I can't see it in your github repo.. |
Done! |
At least here |
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
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
Now there is also a warning about an ignored init attribute for top.state:
Starting with commit b66bf8b the |
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.
|
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 bothrom_rdb[1]
androm_rdb[0]
are1'b0
(=register r0). So as I see it{rom_rdb[1],1'b0}
and .rom_rdb[1:0]
should result in2b'00
. The first however (infail.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)The text was updated successfully, but these errors were encountered: