Open David-Durst opened 5 years ago
David-Durst
commented
5 years ago https://github.com/David-Durst/aetherling/blob/2043af4641f5bf46f5dd111d0812acf98f4eb932/tests/helper_test_readyvalid.py#L19 has the fault exmaple. @rsetaluri can explain how to get this example running in fault.
rsetaluri
commented
5 years ago Thanks for documenting all of this @David-Durst. I'll take a look.
David-Durst
commented
5 years ago @dillonhuff What is the plan for fixing this in the CoreIR simulator?
rdaly525
commented
5 years ago @rsetaluri, @dillonhuff: I have a hunch this is due to implementing the simulation step function as
def step():
eval()
flip_clock()
vs:
def step():
flip_clock()
eval()
The former will possibly report outputs appearing on the previous cycle as the current cycle. This is because it actually takes two calls to step() in order to propagate the clock change from the first step()
In general you should not ever be evaluating a clock change at the same time as an input change.
leonardt
commented
5 years ago I think we need to consider why this is happening in both verilator and the coreir simulator. Is this a fundamental issue w.r.t. to cycle accurate vs event based simulation?
It would be worth reporting this issue to the verilator community to see if they have any insight. Ideally we can provide them a simpler way to reproduce, is there a simple verilog circuit we can construct to demonstrate this issue? Maybe we can just splice the subcircuit of interest from the above test case out? The CPP test bench for verilator should be easy enough to write. What are the necessary components required to reproduce the issue?
rsetaluri
commented
5 years ago I tried a simple circuit according to @David-Durst 's description of that slice:
import magma
import mantle
circ = magma.DefineCircuit("top2", "clk", magma.In(magma.Clock), "counter_out", magma.Out(magma.Bit), "counter_reg_out", magma.Out(magma.Bit), "counter_out_and_counter_reg_out", magma.Out(magma.Bit))
counter = mantle.DefineCounterModM(2, 1, cout=False)()
magma.wire(counter.O[0], circ.counter_out)
reg = mantle.DefineRegister(1)()
magma.wire(counter.O, reg.I)
magma.wire(reg.O[0], circ.counter_reg_out)
magma.wire(reg.O[0] & counter.O[0], circ.counter_out_and_counter_reg_out)
magma.EndDefine()And ran it through fault:
import fault
def prnt():
tester.print(circ.counter_out)
tester.print(circ.counter_reg_out)
tester.print(circ.counter_out_and_counter_reg_out)
for i in range(10):
prnt()
tester.step(1)Using both target="coreir" and target="verilator" (except with tester.step(2) -- this is something we also need to synchronize), we get:
top2.counter_out = 0
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 1
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 0
top2.counter_reg_out = 1
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 1
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 0
top2.counter_reg_out = 1
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 1
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 0
top2.counter_reg_out = 1
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 1
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 0
top2.counter_reg_out = 1
top2.counter_out_and_counter_reg_out = 0
top2.counter_out = 1
top2.counter_reg_out = 0
top2.counter_out_and_counter_reg_out = 0I also tried wrapping the following verilog and get the same output:
module modcounter (
clk,
out
);
input clk;
output reg out;
always @(posedge clk) begin
out <= out + 1;
end
endmodule // modcounter
module register (
clk,
in,
out
);
input clk;
input in;
output reg out;
always @(posedge clk) begin
out <= in;
end
endmodule
module top (
clk,
counter_out,
counter_reg_out,
counter_out_and_counter_reg_out
);
input clk;
output wire counter_out;
output wire counter_reg_out;
output wire counter_out_and_counter_reg_out;
modcounter modcounter_inst(clk, counter_out);
register register_inst(clk, counter_out, counter_reg_out);
assign counter_out_and_counter_reg_out = counter_out && counter_reg_out;
endmodule // top@dillonhuff here are the coreir and verilog for delayed buffer that was behaving poorly. The issue was that it's CE and WE ports were alternating. example_bad_delayed_buffer.zip
David-Durst
commented
5 years ago The code at the time of the issue: https://github.com/David-Durst/aetherling/tree/be2105e73d40e4b2168d1492e45a0a5c6f0cc6c6
The commit that fixed the issue: https://github.com/David-Durst/aetherling/commit/8450c2b8beb61008f42633b3212af20c5dbbe489 . This shifted the phase of the inputs to the delayed buffer so that they are on the same cycle.
There is a bug in both the CoreIR simulator (accessed through magma) and the Verilator simulator (accessed through fault) with an and gate that is receiving mismatched signals.
There were two signals being and together: db.WE and db.CE (https://github.com/David-Durst/aetherling/blob/d27f05846583749d05b5d58665950b3129dca559/aetherling/modules/native_linebuffer/two_dimensional_native_linebuffer.py#L320-L321)
The and was being used to drive a RAM WE port (https://github.com/David-Durst/aetherling/blob/d27f05846583749d05b5d58665950b3129dca559/aetherling/modules/delayed_buffer.py#L84).
In the following Aetherling test (https://github.com/David-Durst/aetherling/blob/d27f05846583749d05b5d58665950b3129dca559/tests/helper_test_readyvalid.py#L18-L55) (note the specific commit, Aetherling has since been fixed), a bug in Aetherling caused the two signals to be mismatched by 1 cycle:
Thanks to the help of @THofstee @rdaly525 and @rsetaluri , this image was produced by VCS by compiling the Magma circuit to verilog and recreating the test bench in Verilog.
The Verilator simulator and the CoreIR simulator showed an impossible output: that the result of the &, the signal driving the RAM WE, was high on the correct cycles. It should always have been low. However, the simulators were merging the &'s of the alternating signals to be high. Please see the below text output from the test in helper_test_readyvalid. Look at cycle i18 where RAMCE is high, WDATA is all 1's, but the next cycle RDATA is all 0's still.