`cxxrtl` output port changes value after back to back `.step()`

[rroohhh]

Version

Yosys 0.45 (git sha1 9ed031ddd588442f22be13ce608547a5809b62f0, g++ 13.3.0 -fPIC -O3)

On which OS did this happen?

Linux

Reproduction Steps

running the following script

read_rtlil <<EOF
read_rtlil <<EOF
module \top
  wire $0\client_valid[0:0]
  wire $not$out.v:13$1_Y
  wire \client_valid
  wire \client_valid_next
  wire input 2 \clock
  wire output 1 \data_out
  cell $not $not$out.v:13$1
    parameter \A_SIGNED 0
    parameter \A_WIDTH 1
    parameter \Y_WIDTH 1
    connect \A \client_valid
    connect \Y $not$out.v:13$1_Y
  end
  process $proc$out.v:15$2
    assign $0\client_valid[0:0] \client_valid_next
    sync posedge \clock
      update \client_valid $0\client_valid[0:0]
  end
  connect \data_out \client_valid
  connect \client_valid_next $not$out.v:13$1_Y
end
EOF

debug write_cxxrtl -print-wire-types out.cxx
exec -expect-return 0 -- c++ -I$(yosys-config --datdir)/include/backends/cxxrtl/runtime main.cpp && ./a.out

using a main.cpp containing

#include "cxxrtl/cxxrtl.h"
#include "out.cxx"
#include <cstdio>

auto main() -> int {
  cxxrtl_design::p_top dut;
  dut.step();

  for(int i = 0; i < 5; i++) {
    dut.p_clock.set(false);
    dut.step();

    dut.p_clock.set(true);
    dut.step();
    auto a = dut.p_data__out.get<int>();
    dut.step();
    auto b = dut.p_data__out.get<int>();
    if (a != b) {
      printf("fail\n");
      return 83;
    }
  }
  return 0;
}

You can also find the yosys script aswell as the main.cpp in the attached repro.zip.

For reference, the rtlil is generated from the following verilog file:

module top(data_out, clock);
  reg client_valid;

  input clock;
  wire clock;

  output data_out;
  wire data_out;

  wire client_valid_next;

  assign data_out = client_valid;
  assign client_valid_next  = ~client_valid;

  always @(posedge clock)
    client_valid <= client_valid_next;
endmodule

Expected Behavior

I expect the a.out binary to never print "fail", or expressed differently for the value of a output port to not change between subsequent .step() calls.

Actual Behavior

The value of the output port changes between .step() calls.

[whitequark]

  output data_out;
  wire data_out;
  assign data_out = client_valid;
  assign client_valid_next  = ~client_valid;

Oh. You're taking feedback from your own output. There's a bit of code that special-cases outputs (it was related to reducing the amount of delta cycles if all you have is a model with a bunch of LED outputs), and since normal code rarely does that, it's buggy.

[whitequark]

Try removing this line and seeing if it helps:

https://github.com/YosysHQ/yosys/blob/f04b89972123325dc48704f30e4f118aa24fca05/backends/cxxrtl/cxxrtl_backend.cc#L3127

[rroohhh]

Oh. You're taking feedback from your own output. There's a bit of code that special-cases outputs (it was related to reducing the amount of delta cycles if all you have is a model with a bunch of LED outputs), and since normal code rarely does that, it's buggy.

What do you mean by feedback from your own output? This is just a comb output driven from a internal register. I don't see how there is feedback from my output?

Try removing this line and seeing if it helps:

https://github.com/YosysHQ/yosys/blob/f04b89972123325dc48704f30e4f118aa24fca05/backends/cxxrtl/cxxrtl_backend.cc#L3127

I tried this and it did not change the output. Also I don't quite see how this could change anything, as !module->get_bool_attribute(ID::top) should always be false for a single module (or a flattened hierarchy) and therefore the if never triggering anyways, right?

[whitequark]

This is just a comb output driven from a internal register. I don't see how there is feedback from my output?

Oh, you're right; I misread the code.