Combinational is overly optimistic in equivalent always_comb inferred sensitivities

[mkorbel1]

Describe the bug

In SystemVerilog, the always_comb sensitivities are only signals referenced within the always_comb block. In ROHD, currently, any combinational signal that may modify the input to a Combinational is treated as a sensitivity. When the always_comb is created, it can generate a non-equivalent implementation relative to the ROHD simulation.

When inlineable modules are used, things are generally equivalent thanks to the fix for #158.

However, if the module is not inlined (e.g. from another generator, see steveicarus/iverilog#872) or the module itself is non-inlineable (easy to create with the existing ROHD generator), then the behavior of the simulation in ROHD will not necessarily match the SystemVerilog simulation.

Perhaps a modification from always_comb to always @(list, of, sensitivities) could help, but this has two problems:

1. The sensitivity could be outside of scope of the current module. XMRs might help, but that's messy and tricky to implement.
2. Assignments and blocks in other locations (other than the "current" always_comb) can be treated as separate processes by the SystemVerilog simulator and lead to non-deterministic race conditions in simulation behavior.

To Reproduce

This new test (based off others in comb_math_test.dart) will fail due to simulation behavior mismatch in ROHD and Icarus Verilog because the & gate has been moved into a non-inlineable module.

class Ander extends Module {
  Logic get c => output('c');
  Ander(Logic a, Logic b) {
    a = addInput('a', a, width: a.width);
    b = addInput('b', b, width: b.width);
    addOutput('c', width: a.width) <= a & b;
  }
}

class ExtraModuleSensitivitySimplerExample extends Module {
  Logic get b => output('b');
  ExtraModuleSensitivitySimplerExample(Logic a) {
    a = addInput('a', a, width: 8);
    addOutput('b', width: 8);

    final inner = Logic(name: 'inner', width: 8);

    Combinational([
      inner < 0xf,
      b < Ander(a, inner).c,
      inner < 0,
    ]);
  }
}

void main() {
  test('extra module sensitivity simpler example', () async {
    final a = Logic(name: 'a', width: 8);
    final mod = ExtraModuleSensitivitySimplerExample(a);
    await mod.build();

    final vectors = [
      Vector({'a': 0xff}, {'b': bin('00001111')})
    ];
    await SimCompare.checkFunctionalVector(mod, vectors);
    final simResult = SimCompare.iverilogVector(mod, vectors);
    expect(simResult, equals(true));
  });
}

Expected behavior

ROHD should reliable generate SystemVerilog which unambiguously has equivalent simulation and synthesis behavior to the simulation behavior in the ROHD simulator.

Actual behavior

ROHD and SV simulators disagree due to extra sensitivities relative to SV and/or nondeterministic race conditions in the SV implementation.

Additional: Dart SDK info

No response

Additional: pubspec.yaml

No response

Additional: Context

Present in ROHD v0.4.2

[mkorbel1]

One possible solution is to make certain types of usage of Combinational illegal in ROHD to help guarantee safety in generated code.

A potential rule:

• "No write after read" - No ConditionalAssignment execution may trigger a glitch upon the receiver of the current or any previous conditional assignment of the current execution of the Combinational.

I'm not sure how to prove it, but I think a rule like this should prevent "bad" scenarios where sensitivities may be masked by multiple writes to the same signal during procedural execution of a Combinational or always_comb.

As described, this would be a simulation-time check, rather than a static check at build time, since statically analyzing for this type of issue would require comprehending the possible execution paths of the Conditional objects, which could be quite tricky. But on the other hand, this allows for generation of even some potentially dangerous SystemVerilog (call it added flexibility) without the risk of simulation-behavior mismatch between a SystemVerilog simulator and the ROHD simulator.

One potential simulation benefit of this is that the search for additional sensitivities in Combinational may not be required anymore.

This restriction means that in a Combinational block, without extra work, doing something like x = x + 1 would be illegal. Perhaps it should be.

The Pipeline abstraction relies on flexibility of reassigning values after reads to get some of it's benefit. Since pipelined variables are accessed through a get operation on the PipelineInfo object, an opportunity presents itself to map signals returned by get to a type of SSA scheme. Thus the example of something like this:

x = x + 1
x = x + 1
x = x + 1

Could be enabled by generating something like this under the hood:

x1 = x0 + 1
x2 = x1 + 1
x3 = x2 + 1
x = x3

Which meets the requirements of the new proposed rule and guarantees safer generated code as well.

[mkorbel1]

The SSA solution works, see #344