Gates should output `X` (never `Z`) when inputs are invalid

[mkorbel1]

Describe the bug

Some gates (e.g. the Mux) will potentially output a Z if the input (e.g. the control) is a Z instead of outputting an X. We should fix the Mux and also search for any other gates or built-in modules that may be outputting a Z instead of an X when inputs are invalid.

To Reproduce

Set the control on Mux to Z, and observe output value.

Expected behavior

Output is X

Actual behavior

Output is Z

Additional: Dart SDK info

No response

Additional: pubspec.yaml

No response

Additional: Context

No response

[dmetis]

I can take a stab at this issue.

[chykon]

Is it because of this problem that the initial value of Logic is Z and not X (dartpad)?

[mkorbel1]

I can take a stab at this issue.

Thanks! It's worth thinking through if there is a methodology for finding all cases where this bug may exist. I don't have a great idea that immediately comes to mind beyond manual inspection or more exhaustive testing.

Is it because of this problem that the initial value of Logic is Z and not X (dartpad)?

This is expected behavior. The z indicates floating, whereas x indicates contention or some other illegal/unpredictable value. Since a Logic not connected to anything is indeed just floating, z seems right. When a gate has floating inputs, however, the output is driven by the gate, just to an unpredictable value, so it should be x. Differentiating helps in debug to find out if a signal is disconnected from any logic or if some invalid scenario has generated an x (including floating inputs to logic).

[chykon]

This is expected behavior. The z indicates floating, whereas x indicates contention or some other illegal/unpredictable value. Since a Logic not connected to anything is indeed just floating, z seems right. When a gate has floating inputs, however, the output is driven by the gate, just to an unpredictable value, so it should be x. Differentiating helps in debug to find out if a signal is disconnected from any logic or if some invalid scenario has generated an x (including floating inputs to logic).

Thanks for clarifying! Does this cause problems with SystemVerilog defaulting to

• x for logic
• z for wire

If I don't confuse anything.

[mkorbel1]

Oh, I didn't know that. Do you know the motivation for that in SystemVerilog? It seems unintuitive to me that an unconnected signal would be x instead of z since z is floating.

[chykon]

Sorry, I'm not very familiar with SystemVerilog, so I can't explain the motivation. Perhaps this is specific to the fact that SystemVerilog has different partially interchangeable data types (logic, reg, wire and others), as opposed to the cleaner design of ROHD. After your explanation, I also think that ROHD is taking a more logical approach.

I think this inconsistency can be especially pronounced during cosimulation.

[mkorbel1]

Since x and z are simulation constructs only, I think I'm not too concerned by the inconsistency here. Especially since they are both considered invalid values in simulation anyways.

I think even in cosimulation propagating x vs z between simulators will be handled "correctly", even if their sources may be inconsistent.

Thanks for raising it, though!

[chykon]

As far as I understood, this code also reproduces this problem:

import 'dart:math';
import 'package:rohd/rohd.dart';

enum MCUInterfaceTag { input, output }

class MCUInterface extends Interface<MCUInterfaceTag> {
  MCUInterface({this.smallMemory = false}) {
    setPorts([
      Port('clock'),
      Port('enable'),
      Port('write'),
      Port('selectByte'),
      Port('address', 16),
      Port('inputData', 16),
    ], [
      MCUInterfaceTag.input
    ]);

    setPorts([
      Port('outputData', 16),
    ], [
      MCUInterfaceTag.output
    ]);
  }

  Logic get clock => port('clock');
  Logic get enable => port('enable');
  Logic get write => port('write');
  Logic get selectByte => port('selectByte');
  Logic get address => port('address');
  Logic get inputData => port('inputData');

  Logic get outputData => port('outputData');

  final bool smallMemory;
}

class MemoryControllerUnit extends Module {
  MemoryControllerUnit(MCUInterface intf) {
    this.intf = MCUInterface(smallMemory: intf.smallMemory)
      ..connectIO(this, intf,
          inputTags: {MCUInterfaceTag.input},
          outputTags: {MCUInterfaceTag.output});

    _buildLogic();
  }

  void _buildLogic() {
    memory = <Logic>[];
    for (var i = 0;
        i < (intf.smallMemory ? 256 : pow(2, intf.address.width));
        ++i) {
      memory.add(Logic(name: 'memoryElement$i', width: 8));
    }

    final writeByteCaseItems = <CaseItem>[];
    final readByteCaseItems = <CaseItem>[];
    final writeHalfwordCaseItems = <CaseItem>[];
    final readHalfwordCaseItems = <CaseItem>[];
    for (var i = 0; i < memory.length; ++i) {
      writeByteCaseItems.add(CaseItem(Const(i, width: intf.address.width),
          [memory[i] < intf.inputData.slice(7, 0)]));
      readByteCaseItems.add(CaseItem(Const(i, width: intf.address.width),
          [intf.outputData < intf.outputData.withSet(0, memory[i])]));

      if (i.isEven) {
        writeHalfwordCaseItems
            .add(CaseItem(Const(i, width: intf.address.width), [
          memory[i] < intf.inputData.slice(7, 0),
          memory[i + 1] < intf.inputData.slice(15, 8)
        ]));
        readHalfwordCaseItems
            .add(CaseItem(Const(i, width: intf.address.width), [
          intf.outputData < [memory[i + 1], memory[i]].swizzle()
        ]));
      }
    }

    Sequential(intf.clock, [
      If(intf.enable, then: [
        If(intf.write, then: [
          If(intf.selectByte,
              then: [Case(intf.address, writeByteCaseItems)],
              orElse: [Case(intf.address, writeHalfwordCaseItems)])
        ], orElse: [
          If(intf.selectByte,
              then: [Case(intf.address, readByteCaseItems)],
              orElse: [Case(intf.address, readHalfwordCaseItems)])
        ])
      ])
    ]);
  }

  late final MCUInterface intf;
  late final List<Logic> memory;
}

void main() async {
  final mcu = MCUInterface(smallMemory: true);
  mcu.clock <= SimpleClockGenerator(10).clk;

  final mcuInstance = MemoryControllerUnit(mcu);
  await mcuInstance.build();

  WaveDumper(mcuInstance);

  Simulator.registerAction(10, () {
    mcu.enable.put(1);
    mcu.write.put(1);
    mcu.selectByte.put(0);
    mcu.address.put(2);
    mcu.inputData.put(258);
  });

  Simulator.registerAction(20, () {
    mcu.write.put(0);
    mcu.inputData.put(3);
  });

  Simulator.registerAction(30, () {
    mcu.enable.put(0);
    mcu.write.put(1);
    mcu.selectByte.put(1);
    mcu.address.put(1);
  });

  Simulator.registerAction(40, () {
    mcu.enable.put(1);
  });

  Simulator.registerAction(50, () {
    mcu.write.put(0);
  });

  Simulator.setMaxSimTime(100);
  await Simulator.run();
}

Screenshot:

[mkorbel1]

As far as I understood, this code also reproduces this problem

Yes, I think you're right, it looks like Case has the same issue. Thanks @chykon!

[chykon]

However, if you connect clock AFTER the module is instantiated, the output signals are managed correctly.

final mcu = MCUInterface(smallMemory: true);
final mcuInstance = MemoryControllerUnit(mcu);
mcu.clock <= SimpleClockGenerator(10).clk;

await mcuInstance.build();

[chykon]

And here is another example. First x appears in the outputs, and then z. I think this is due to the fact that in this example the memory elements have connections to each other, unlike the first example.

import 'dart:math';
import 'package:rohd/rohd.dart';

enum RFCUInterfaceTag { input, output }

class RFCUInterface extends Interface<RFCUInterfaceTag> {
  RFCUInterface() {
    setPorts([
      Port('clock'),
      Port('enable'),
      Port('transferMode', 2),
      Port('source', 8),
      Port('destination', 8),
    ], [
      RFCUInterfaceTag.input
    ]);

    setPorts([
      Port('data', 8),
    ], [
      RFCUInterfaceTag.output
    ]);
  }

  Logic get clock => port('clock');
  Logic get enable => port('enable');
  Logic get transferMode => port('transferMode');
  Logic get source => port('source');
  Logic get destination => port('destination');

  Logic get data => port('data');

  static const transferModeInternal = 0;
  static const transferModeExternalSource = 1;
  static const transferModeExternalDestination = 2;
}

class RegisterFileControllerUnit extends Module {
  RegisterFileControllerUnit(RFCUInterface intf) {
    this.intf = RFCUInterface()
      ..connectIO(this, intf,
          inputTags: {RFCUInterfaceTag.input},
          outputTags: {RFCUInterfaceTag.output});

    _buildLogic();
  }

  void _buildLogic() {
    registers = <Logic>[];
    for (var i = 0; i < pow(2, _addressWidth); ++i) {
      registers.add(Logic(name: 'register$i', width: 8));
    }

    final internalSourceCaseItems = <CaseItem>[];
    final externalSourceCaseItems = <CaseItem>[];
    final externalDestinationCaseItems = <CaseItem>[];
    for (var i = 0; i < registers.length; ++i) {
      final internalDestinationCaseItems = <CaseItem>[];
      for (var j = 0; j < registers.length; ++j) {
        if (j == i) {
          continue;
        }
        internalDestinationCaseItems.add(CaseItem(
            Const(j, width: _addressWidth), [registers[j] < registers[i]]));
      }
      internalSourceCaseItems.add(CaseItem(Const(i, width: _addressWidth),
          [Case(intf.destination.slice(6, 0), internalDestinationCaseItems)]));

      externalSourceCaseItems.add(CaseItem(
          Const(i, width: _addressWidth), [registers[i] < intf.source]));

      externalDestinationCaseItems.add(
          CaseItem(Const(i, width: _addressWidth), [intf.data < registers[i]]));
    }

    Sequential(intf.clock, [
      If(intf.enable, then: [
        IfBlock([
          Iff.s(intf.transferMode.eq(RFCUInterface.transferModeInternal),
              Case(intf.source.slice(6, 0), internalSourceCaseItems)),
          ElseIf.s(
              intf.transferMode.eq(RFCUInterface.transferModeExternalSource),
              Case(intf.destination.slice(6, 0), externalSourceCaseItems)),
          ElseIf.s(
              intf.transferMode
                  .eq(RFCUInterface.transferModeExternalDestination),
              Case(intf.source.slice(6, 0), externalDestinationCaseItems))
        ])
      ])
    ]);
  }

  late final RFCUInterface intf;
  late final List<Logic> registers;

  static const _addressWidth = 7;
}

void main() async {
  final rfcu = RFCUInterface();
  final rfcuInstance = RegisterFileControllerUnit(rfcu);
  rfcu.clock <= SimpleClockGenerator(10).clk;

  await rfcuInstance.build();

  WaveDumper(rfcuInstance);

  Simulator.registerAction(30, () {
    rfcu.transferMode.put(RFCUInterface.transferModeExternalSource);
    rfcu.source.put(255);
    rfcu.destination.put(3);
  });

  Simulator.registerAction(40, () {
    rfcu.enable.put(0);
  });

  Simulator.registerAction(50, () {
    rfcu.enable.put(1);
  });

  Simulator.registerAction(60, () {
    rfcu.transferMode.put(RFCUInterface.transferModeInternal);
    rfcu.source.put(3);
    rfcu.destination.put(0);
  });

  Simulator.registerAction(70, () {
    rfcu.transferMode.put(RFCUInterface.transferModeExternalDestination);
    rfcu.source.put(0);
  });

  Simulator.registerAction(80, () {
    rfcu.transferMode.put(RFCUInterface.transferModeInternal);
    rfcu.source.put(5);
    rfcu.destination.put(6);
  });

  Simulator.setMaxSimTime(100);
  await Simulator.run();
}

[mkorbel1]

These seem to be symptoms of the bug being in the ConditionalAssignment itself. Hopefully fixing that will fix all of these cases, but these are good examples for adding unit tests, thank you!