cxxrtl: -O1 optimisation unbuffers wires too permissively

[toby-bro]

Version

Yosys 0.57+148 (git sha1 259bd6f, g++ 15.2.1 -march=x86-64 -mtune=generic -O2 -fno-plt -fexceptions -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -flto=auto -fPIC -O3)

On which OS did this happen?

Linux

Reproduction Steps

When running this script (which creates a systemverilog file, it's testbench then runs them),

#!/bin/bash

rm -f m_feat2_simplified.sv m_feat2_simplified.cc testbench.cpp sim_O0 sim_O1

cat > m_feat2_simplified.sv << 'EOF'
module m_feat2_simplified (
    input logic clk,
    input wire rst,
    input logic [7:0] v_in,
    output logic [7:0] v_out
);
    int counter;

    task incr(); counter++; endtask
    initial begin
        counter = 0;
    end
    always_ff @(posedge clk) begin
        incr();
        v_out <= v_in ^ counter[7:0];
    end
endmodule
EOF

cat > testbench.cpp << 'EOF'
#include "m_feat2_simplified.cc"

#include <iostream>
#include <iomanip>

int main() {
    cxxrtl_design::p_m__feat2__simplified dut;
    

    dut.p_v__in.set<uint8_t>(0xFF);  // v_in = ff (from bug report)
    dut.p_rst.set<bool>(false);      // rst = 0 (from bug report)
    dut.p_clk.set<bool>(true);       // clk = 1 (from bug report)
    

    dut.step();
    

    for (int cycle = 0; cycle < 50; cycle++) {
        dut.p_clk.set<bool>(false);
        dut.step();
        dut.p_clk.set<bool>(true); 
        dut.step();
    }
    

    for (int settle = 0; settle < 20; settle++) {
        dut.step();
    }
    

    uint8_t result = dut.p_v__out.get<uint8_t>();
    for (int i = 7; i >= 0; i--) {
        std::cout << ((result >> i) & 1);
    }
    std::cout << std::endl;
    
    return 0;
}
EOF

for opt in 0 1; do
    echo "Testing with -O$opt:"

    yosys -q -p "read_verilog -sv m_feat2_simplified.sv; prep -top m_feat2_simplified ; write_cxxrtl -O$opt m_feat2_simplified.cc"

    g++ -std=c++17 -O0 -I$(yosys-config --datdir)/include/backends/cxxrtl/runtime testbench.cpp -I. -o sim_O$opt
    ./sim_O$opt
done

Expected Behavior

The two results with optimisation level -O0 and -O1 are different

Actual Behavior

I would have expected the results of both tests to be the same...
Especially I agree with the first result with -O0 rather than with that of -O1 (from having simulated this sv design on other simulators)

[whitequark]

I would have expected the results of both tests to be the same...

Note that CXXRTL does not guarantee equivalency of generated across different build options if the input contains race conditions. I don't see where the race is on a very cursory look though, so it might be a CXXRTL bug.

Is your goal to simulate a netlist? In that case I recommend using the highest optimization level possible. You aren't losing any debug information, the compilation times may well get lower, and certain kinds of race conditions coincidentally happen to be eliminated.

[toby-bro]

I checked the diff between the two generated files.
It seems as if the "unbuffering of internal wires" is changing the order of the operations in the sequential always_ff block.
The problematic difference in the cxxrtl generated files is

bool p_m__feat2__simplified::eval(performer *performer) {
	bool converged = false;
	bool posedge_p_clk = this->posedge_p_clk();
	// \src: m_feat2_simplified.sv:9.17-9.27
	// cell $add$m_feat2_simplified.sv:9$3
	i_0_5c_counter_5b_31_3a_0_5d_.next = add_ss<32>(p_counter.curr, value<2>{0x1u});
	// \src: m_feat2_simplified.sv:15.18-15.37
	// cell $xor$m_feat2_simplified.sv:15$4
	i_0_5c_v__out_5b_7_3a_0_5d_.next = xor_uu<8>(p_v__in.curr, i_0_5c_counter_5b_31_3a_0_5d_.curr.slice<7,0>().val());
	// \always_ff: 1
	// \src: m_feat2_simplified.sv:13.5-16.8
	// cell $procdff$7
	if (posedge_p_clk) {
		p_counter.next = i_0_5c_counter_5b_31_3a_0_5d_.curr;
	}
	// \always_ff: 1
	// \src: m_feat2_simplified.sv:13.5-16.8
	// cell $procdff$6
	if (posedge_p_clk) {
		p_v__out.next = i_0_5c_v__out_5b_7_3a_0_5d_.curr;
	}
	return converged;
}

becomes

bool p_m__feat2__simplified::eval(performer *performer) {
	bool converged = true;
	bool posedge_p_clk = this->posedge_p_clk();
	// \src: m_feat2_simplified.sv:9.17-9.27
	// cell $add$m_feat2_simplified.sv:9$3
	i_0_5c_counter_5b_31_3a_0_5d_ = add_ss<32>(p_counter.curr, value<2>{0x1u});
	// \src: m_feat2_simplified.sv:15.18-15.37
	// cell $xor$m_feat2_simplified.sv:15$4
	i_0_5c_v__out_5b_7_3a_0_5d_ = xor_uu<8>(p_v__in.curr, i_0_5c_counter_5b_31_3a_0_5d_.slice<7,0>().val());
	// \always_ff: 1
	// \src: m_feat2_simplified.sv:13.5-16.8
	// cell $procdff$7
	if (posedge_p_clk) {
		p_counter.next = i_0_5c_counter_5b_31_3a_0_5d_;
	}
	// \always_ff: 1
	// \src: m_feat2_simplified.sv:13.5-16.8
	// cell $procdff$6
	if (posedge_p_clk) {
		p_v__out.next = i_0_5c_v__out_5b_7_3a_0_5d_;
	}
	return converged;
}

And so (from what I understand) the i_0_5c_counter_5b_31_3a_0_5d_ wins an extra clock cycle in the xor_uu because instead of using it's curr value it uses it's next value straightaway if we stay in the perspective of wires instead of values.

[toby-bro]

I would think that the bug is in lines

yosys/backends/cxxrtl/cxxrtl_backend.cc

Lines 3139 to 3156 in e546f3b

	for (auto wire : module->wires()) {
	auto &wire_type = wire_types[wire];
	wire_type = {WireType::BUFFERED};
	if (feedback_wires[wire]) continue;
	if (wire->port_output && !module->get_bool_attribute(ID::top)) continue;
	if (!wire->name.isPublic() && !unbuffer_internal) continue;
	if (wire->name.isPublic() && !unbuffer_public) continue;
	if (flow.wire_sync_defs.count(wire) > 0) continue;
	wire_type = {WireType::MEMBER};
	if (edge_wires[wire]) continue;
	if (wire->get_bool_attribute(ID::keep)) continue;
	if (wire->port_input || wire->port_output) continue;
	if (!wire->name.isPublic() && !localize_internal) continue;
	if (wire->name.isPublic() && !localize_public) continue;
	wire_type = {WireType::LOCAL};
	}

As it does not check if the wire it is unbuffering has delta cycle dependencies (if it is a source and destination in the same cycle)

[whitequark]

As it does not check if the wire it is unbuffering has delta cycle dependencies (if it is a source and destination in the same cycle)

I still haven't had time to check this issue in depth (sorry!) but if this is indeed what's happening here then I agree CXXRTL has a bug.