From 160eb5cd06ee449b7242f6a935c041eacf45e314 Mon Sep 17 00:00:00 2001 From: Max Korbel Date: Fri, 14 Jul 2023 12:47:36 -0700 Subject: [PATCH] Fix bugs where SSA could potentially generate inferred latches (#391) --- .github/configs/mlc_config.json | 3 + .github/workflows/general.yml | 2 +- CODE_OF_CONDUCT.md | 2 +- lib/src/modules/conditional.dart | 94 ++++++++++++++++--- test/ssa_test.dart | 149 ++++++++++++++++++++++++++++--- 5 files changed, 224 insertions(+), 26 deletions(-) diff --git a/.github/configs/mlc_config.json b/.github/configs/mlc_config.json index 419310c1b..97369bb40 100644 --- a/.github/configs/mlc_config.json +++ b/.github/configs/mlc_config.json @@ -2,6 +2,9 @@ "ignorePatterns": [ { "pattern": "^https://www.cadence.com/en_US/home/tools/digital-design-and-signoff/synthesis/stratus-high-level-synthesis.html$" + }, + { + "pattern":"^https://www.intel.com/content/www/us/en/security-center/vulnerability-handling-guidelines.html$" } ] } diff --git a/.github/workflows/general.yml b/.github/workflows/general.yml index 060600cda..c87c32aaf 100644 --- a/.github/workflows/general.yml +++ b/.github/workflows/general.yml @@ -18,7 +18,7 @@ jobs: uses: actions/checkout@v3 - name: Lint Markdown files - uses: DavidAnson/markdownlint-cli2-action@v9 + uses: DavidAnson/markdownlint-cli2-action@v11 with: globs: '**/*.md' diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md index 2523a4f2f..b1480413e 100644 --- a/CODE_OF_CONDUCT.md +++ b/CODE_OF_CONDUCT.md @@ -61,7 +61,7 @@ representative at an online or offline event. Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at -CommunityCodeofConduct@intel.com. +. All complaints will be reviewed and investigated promptly and fairly. All community leaders are obligated to respect the privacy and security of the diff --git a/lib/src/modules/conditional.dart b/lib/src/modules/conditional.dart index 8c46498f6..b51579445 100644 --- a/lib/src/modules/conditional.dart +++ b/lib/src/modules/conditional.dart @@ -10,6 +10,7 @@ import 'dart:async'; import 'dart:collection'; +import 'package:collection/collection.dart'; import 'package:meta/meta.dart'; import 'package:rohd/rohd.dart'; import 'package:rohd/src/collections/duplicate_detection_set.dart'; @@ -645,6 +646,11 @@ abstract class Conditional { @Deprecated('Use `receivers` instead.') List getReceivers() => receivers; + /// The same as [receivers], but uncached for situations where the list of + /// [conditionals] may still be modified or to compute the cached result + /// for [receivers] itself. + List _getReceivers(); + /// Lists *all* receivers, recursively including all sub-[Conditional]s /// receivers. List get receivers; @@ -767,9 +773,11 @@ class ConditionalGroup extends Conditional { ]; @override - late final List receivers = [ - for (final conditional in conditionals) ...conditional.receivers - ]; + late final List receivers = _getReceivers(); + + @override + List _getReceivers() => + [for (final conditional in conditionals) ...conditional.receivers]; @override void execute(Set drivenSignals, void Function(Logic toGuard)? guard) { @@ -815,6 +823,9 @@ class ConditionalAssign extends Conditional { @override late final List receivers = [receiver]; + @override + List _getReceivers() => receivers; + @override late final List drivers = [driver]; @@ -917,7 +928,8 @@ class Case extends Conditional { final List items; /// The default to execute when there was no match with any other [CaseItem]s. - final List? defaultItem; + List? get defaultItem => _defaultItem; + List? _defaultItem; /// The type of case block this is, for special attributes /// (e.g. [ConditionalType.unique], [ConditionalType.priority]). @@ -929,7 +941,9 @@ class Case extends Conditional { /// /// If none of [items] match, then [defaultItem] is executed. Case(this.expression, this.items, - {this.defaultItem, this.conditionalType = ConditionalType.none}) { + {List? defaultItem, + this.conditionalType = ConditionalType.none}) + : _defaultItem = defaultItem { for (final item in items) { if (item.value.width != expression.width) { throw PortWidthMismatchException.equalWidth(expression, item.value); @@ -1008,7 +1022,7 @@ class Case extends Conditional { /// Calculates the set of conditionals directly within this. List _getConditionals() => [ ...items.map((item) => item.then).expand((conditional) => conditional), - ...defaultItem ?? [] + if (defaultItem != null) ...defaultItem! ]; @override @@ -1037,19 +1051,19 @@ class Case extends Conditional { @override late final List receivers = _getReceivers(); - /// Calculates the set of receivers recursively down. + @override List _getReceivers() { final receivers = []; for (final item in items) { receivers.addAll(item.then .map((conditional) => conditional.receivers) - .expand((receiver) => receiver) + .flattened .toList(growable: false)); } if (defaultItem != null) { receivers.addAll(defaultItem! .map((conditional) => conditional.receivers) - .expand((receiver) => receiver) + .flattened .toList(growable: false)); } return receivers; @@ -1099,6 +1113,9 @@ ${subPadding}end @override Map _processSsa(Map currentMappings, {required int context}) { + // add an empty default if there isn't already one, since we need it for phi + _defaultItem ??= []; + // first connect direct drivers into the case statement Conditional._connectSsaDriverFromMappings(expression, currentMappings, context: context); @@ -1111,7 +1128,7 @@ ${subPadding}end final phiMappings = {}; for (final conditionals in [ ...items.map((e) => e.then), - if (defaultItem != null) defaultItem!, + defaultItem!, ]) { var localMappings = {...currentMappings}; @@ -1134,6 +1151,30 @@ ${subPadding}end final newMappings = {...currentMappings}..addAll(phiMappings); + // find all the SSA signals that are driven by anything in this case block, + // since we need to ensure every case drives them or else we may create + // an inferred latch + final signalsNeedingInit = [ + ...items.map((e) => e.then).flattened, + ...defaultItem!, + ].map((e) => e._getReceivers()).flattened.whereType<_SsaLogic>().toSet(); + for (final conditionals in [ + ...items.map((e) => e.then), + defaultItem!, + ]) { + final alreadyDrivenSsaSignals = conditionals + .map((e) => e._getReceivers()) + .flattened + .whereType<_SsaLogic>() + .toSet(); + + for (final signalNeedingInit in signalsNeedingInit) { + if (!alreadyDrivenSsaSignals.contains(signalNeedingInit)) { + conditionals.add(signalNeedingInit < 0); + } + } + } + return newMappings; } } @@ -1240,7 +1281,7 @@ class If extends Conditional { If(Logic condition, {List? then, List? orElse}) : this.block([ Iff(condition, then ?? []), - Else(orElse ?? []), + if (orElse != null) Else(orElse), ]); /// If [condition] is high, then [then] is excutes, @@ -1327,7 +1368,7 @@ class If extends Conditional { @override late final List receivers = _getReceivers(); - /// Calculates the set of receivers recursively down. + @override List _getReceivers() { final receivers = []; for (final iff in iffs) { @@ -1370,6 +1411,11 @@ ${padding}end '''); @override Map _processSsa(Map currentMappings, {required int context}) { + // add an empty else if there isn't already one, since we need it for phi + if (iffs.last is! Else) { + iffs.add(Else([])); + } + // first connect direct drivers into the if statements for (final iff in iffs) { Conditional._connectSsaDriverFromMappings(iff.condition, currentMappings, @@ -1400,6 +1446,30 @@ ${padding}end '''); final newMappings = {...currentMappings}..addAll(phiMappings); + // find all the SSA signals that are driven by anything in this if block, + // since we need to ensure every case drives them or else we may create + // an inferred latch + final signalsNeedingInit = iffs + .map((e) => e.then) + .flattened + .map((e) => e._getReceivers()) + .flattened + .whereType<_SsaLogic>() + .toSet(); + for (final iff in iffs) { + final alreadyDrivenSsaSignals = iff.then + .map((e) => e._getReceivers()) + .flattened + .whereType<_SsaLogic>() + .toSet(); + + for (final signalNeedingInit in signalsNeedingInit) { + if (!alreadyDrivenSsaSignals.contains(signalNeedingInit)) { + iff.then.add(signalNeedingInit < 0); + } + } + } + return newMappings; } } diff --git a/test/ssa_test.dart b/test/ssa_test.dart index 890c9a119..a880a8e88 100644 --- a/test/ssa_test.dart +++ b/test/ssa_test.dart @@ -228,7 +228,7 @@ class SsaNested extends SsaTestModule { } class SsaMultiDep extends SsaTestModule { - SsaMultiDep(Logic a) : super(name: 'nested') { + SsaMultiDep(Logic a) : super(name: 'multidep') { a = addInput('a', a, width: 8); final x = addOutput('x', width: 8); final y = addOutput('y', width: 8); @@ -250,23 +250,93 @@ class SsaMultiDep extends SsaTestModule { int model(int a) => a + 3; } +class SsaSequenceOfIfs extends Module { + SsaSequenceOfIfs(Logic a, Logic reset) : super(name: 'seqofifs') { + final incr = addInput('a', a, width: 8)[0]; + reset = addInput('reset', reset); + final decr = Const(0); + final x = addOutput('x', width: 8); + + final val = Logic(width: 8)..put(0); // this is ok for this test... + final nextVal = Logic(width: 8); + + final checkVal = Const(3, width: 4).zeroExtend(8); + + Combinational.ssa((s) => [ + s(nextVal) < val, + If(incr, then: [nextVal.incr(s: s)]), + If(decr & (s(nextVal) > 0), then: [ + nextVal.decr(s: s), + ]), + If(s(nextVal) > checkVal, then: [ + s(nextVal) < checkVal, + ]) + ]); + + Sequential(SimpleClockGenerator(10).clk, reset: reset, [ + val < nextVal, + ]); + + x <= (nextVal > 0).zeroExtend(8); + } +} + +class SsaSequenceOfCases extends Module { + SsaSequenceOfCases(Logic a, Logic reset) : super(name: 'seqofifs') { + final incr = addInput('a', a, width: 8)[0]; + reset = addInput('reset', reset); + final decr = Const(0); + final x = addOutput('x', width: 8); + + final val = Logic(width: 8)..put(0); // this is ok for this test... + final nextVal = Logic(width: 8); + + final checkVal = Const(3, width: 4).zeroExtend(8); + + Combinational.ssa((s) => [ + s(nextVal) < val, + Case(Const(1), [ + CaseItem(incr, [ + nextVal.incr(s: s), + ]) + ]), + Case(Const(1), [ + CaseItem(decr & (s(nextVal) > 0), [ + nextVal.decr(s: s), + ]) + ]), + Case(Const(1), [ + CaseItem(s(nextVal) > checkVal, [ + s(nextVal) < checkVal, + ]) + ]), + ]); + + Sequential(SimpleClockGenerator(10).clk, reset: reset, [ + val < nextVal, + ]); + + x <= (nextVal > 0).zeroExtend(8); + } +} + void main() { tearDown(() async { await Simulator.reset(); }); - final aInput = Logic(width: 8, name: 'a'); - final mods = [ - SsaModAssignsOnly(aInput), - SsaModIf(aInput), - SsaModCase(aInput), - SsaChain(aInput), - SsaMix(aInput), - SsaNested(aInput), - SsaMultiDep(aInput), - ]; - group('ssa_test_module', () { + final aInput = Logic(width: 8, name: 'a'); + final mods = [ + SsaModAssignsOnly(aInput), + SsaModIf(aInput), + SsaModCase(aInput), + SsaChain(aInput), + SsaMix(aInput), + SsaNested(aInput), + SsaMultiDep(aInput), + ]; + for (final mod in mods) { test('ssa ${mod.name}', () async { await mod.build(); @@ -281,6 +351,61 @@ void main() { } }); + test('ssa seq of ifs', () async { + final mod = SsaSequenceOfIfs(Logic(width: 8), Logic()); + await mod.build(); + + final vectors = [ + Vector({'a': 0, 'reset': 1}, {}), + Vector({'a': 0, 'reset': 0}, {}), + Vector({'a': 0}, {'x': 0}), + Vector({'a': 0}, {'x': 0}), + Vector({'a': 1}, {'x': 1}), + Vector({'a': 1}, {'x': 1}), + Vector({'a': 1}, {'x': 1}), + ]; + + // make sure we don't have any inferred latches (X's) + for (final signal in mod.signals) { + signal.changed.listen((event) { + expect(event.newValue.isValid, isTrue); + }); + } + Simulator.registerAction(15, () { + for (final signal in mod.signals) { + expect(signal.value.isValid, isTrue); + } + }); + + await SimCompare.checkFunctionalVector(mod, vectors); + SimCompare.checkIverilogVector(mod, vectors); + }); + + test('ssa seq of cases', () async { + final mod = SsaSequenceOfCases(Logic(width: 8), Logic()); + await mod.build(); + + final vectors = [ + Vector({'a': 0, 'reset': 1}, {}), + Vector({'a': 0, 'reset': 0}, {}), + Vector({'a': 0}, {'x': 0}), + Vector({'a': 0}, {'x': 0}), + Vector({'a': 1}, {'x': 1}), + Vector({'a': 1}, {'x': 1}), + Vector({'a': 1}, {'x': 1}), + ]; + + // make sure we don't have any inferred latches (X's) + Simulator.registerAction(15, () { + for (final signal in mod.signals) { + expect(signal.value.isValid, isTrue); + } + }); + + await SimCompare.checkFunctionalVector(mod, vectors); + SimCompare.checkIverilogVector(mod, vectors); + }); + test('ssa uninitialized', () async { expect(() => SsaUninit(Logic(width: 8)), throwsA(isA()));