From 4a54807ff56610db239ac9ca81dde245339cca05 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Mon, 8 May 2023 13:43:50 -0600 Subject: [PATCH 01/11] [FIRRTL] DropConst pass Added DropConst pass that checks const usage in when blocks, then drops all const modifiers from types. --- include/circt/Dialect/FIRRTL/FIRRTLTypes.h | 3 + .../circt/Dialect/FIRRTL/FIRRTLTypesImpl.td | 19 +- include/circt/Dialect/FIRRTL/FIRRTLVisitors.h | 3 +- include/circt/Dialect/FIRRTL/Passes.h | 2 + include/circt/Dialect/FIRRTL/Passes.td | 17 +- lib/Dialect/FIRRTL/FIRRTLTypes.cpp | 44 +++++ lib/Dialect/FIRRTL/Transforms/CMakeLists.txt | 1 + lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 184 ++++++++++++++++++ lib/Firtool/Firtool.cpp | 2 + test/Dialect/FIRRTL/drop-const-errors.mlir | 40 ++++ test/Dialect/FIRRTL/drop-const.mlir | 138 +++++++++++++ 11 files changed, 445 insertions(+), 8 deletions(-) create mode 100644 lib/Dialect/FIRRTL/Transforms/DropConst.cpp create mode 100644 test/Dialect/FIRRTL/drop-const-errors.mlir create mode 100644 test/Dialect/FIRRTL/drop-const.mlir diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h index ee73cb71a1a5..6f626835b8d1 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h @@ -135,6 +135,9 @@ class FIRRTLBaseType /// Return a 'const' or non-'const' version of this type. FIRRTLBaseType getConstType(bool isConst); + /// Return this type with a 'const' modifiers dropped + FIRRTLBaseType getAllConstDroppedType(); + /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType getMaskType(); diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td index 201a93981bac..1be237c48b7f 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td @@ -22,12 +22,12 @@ class FIRRTLImplType traits = [], string baseCppClass = "::circt::firrtl::FIRRTLBaseType"> : TypeDef { - // Storage classes must be defined in C++ and + // Storage classes must be defined in C++ and // inherit from FIRRTLBaseTypeStorage let genStorageClass = false; - - // MLIR generates awkward accessor "getIsConst" for the "isConst" parameter, - // which is common on FIRRTLBaseType anyway, so we generate the other + + // MLIR generates awkward accessor "getIsConst" for the "isConst" parameter, + // which is common on FIRRTLBaseType anyway, so we generate the other // accessors manually let genAccessors = false; } @@ -212,6 +212,9 @@ def FVectorImpl : BaseVectorTypeImpl<"FVector","::circt::firrtl::FIRRTLBaseType" let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); + + /// Return this type with a 'const' modifiers dropped + FVectorType getAllConstDroppedType(); }]; } @@ -331,6 +334,9 @@ def BundleImpl : BaseBundleTypeImpl<"Bundle","::circt::firrtl::FIRRTLBaseType", let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); + + /// Return this type with a 'const' modifiers dropped + BundleType getAllConstDroppedType(); }]; } @@ -372,6 +378,9 @@ def FEnumImpl : FIRRTLImplType<"FEnum", [FieldIDTypeInterface]> { FEnumType getConstType(bool isConst); + /// Return this type with a 'const' modifiers dropped + FEnumType getAllConstDroppedType(); + /// Look up an element's index by name. This returns None on failure. std::optional getElementIndex(StringAttr name); std::optional getElementIndex(StringRef name); @@ -450,7 +459,7 @@ def RefImpl : FIRRTLImplType<"Ref", Values of this type are used to capture dataflow paths, and do not represent a circuit element or entity. - + Generally read-only (probe), optionally forceable (rwprobe). }]; let parameters = (ins TypeParameter<"::circt::firrtl::FIRRTLBaseType", diff --git a/include/circt/Dialect/FIRRTL/FIRRTLVisitors.h b/include/circt/Dialect/FIRRTL/FIRRTLVisitors.h index e74a1eb96fff..db552caa8157 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLVisitors.h +++ b/include/circt/Dialect/FIRRTL/FIRRTLVisitors.h @@ -54,7 +54,7 @@ class ExprVisitor { TailPrimOp, VerbatimExprOp, HWStructCastOp, BitCastOp, RefSendOp, RefResolveOp, RefSubOp, // Casts to deal with weird stuff - UninferredResetCastOp, UninferredWidthCastOp, + UninferredResetCastOp, UninferredWidthCastOp, ConstCastOp, mlir::UnrealizedConversionCastOp>([&](auto expr) -> ResultType { return thisCast->visitExpr(expr, args...); }) @@ -174,6 +174,7 @@ class ExprVisitor { HANDLE(HWStructCastOp, Unhandled); HANDLE(UninferredResetCastOp, Unhandled); HANDLE(UninferredWidthCastOp, Unhandled); + HANDLE(ConstCastOp, Unhandled); HANDLE(mlir::UnrealizedConversionCastOp, Unhandled); HANDLE(BitCastOp, Unhandled); #undef HANDLE diff --git a/include/circt/Dialect/FIRRTL/Passes.h b/include/circt/Dialect/FIRRTL/Passes.h index 5dd497660bb2..8c96c192411a 100644 --- a/include/circt/Dialect/FIRRTL/Passes.h +++ b/include/circt/Dialect/FIRRTL/Passes.h @@ -127,6 +127,8 @@ std::unique_ptr createVectorizationPass(); std::unique_ptr createInjectDUTHierarchyPass(); +std::unique_ptr createDropConstPass(); + /// Configure which values will be explicitly preserved by the DropNames pass. namespace PreserveValues { enum PreserveMode { diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 9d767726c87b..2b0a62603b26 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -97,8 +97,8 @@ def IMConstProp : Pass<"firrtl-imconstprop", "firrtl::CircuitOp"> { def RegisterOptimizer : Pass<"firrtl-register-optimizer", "firrtl::FModuleOp"> { let summary = "Optimizer Registers"; let description = [{ - This pass applies classic FIRRTL register optimizations. These - optimizations are isolated to this pass as they can change the visible + This pass applies classic FIRRTL register optimizations. These + optimizations are isolated to this pass as they can change the visible behavior of the register, especially before reset. }]; let constructor = "circt::firrtl::createRegisterOptimizerPass()"; @@ -691,6 +691,19 @@ def VBToBV : Pass<"firrtl-vb-to-bv", "firrtl::CircuitOp"> { let constructor = "circt::firrtl::createVBToBVPass()"; } +def DropConst : InterfacePass<"firrtl-drop-const", "firrtl::FModuleLike"> { + let summary = "Check usage of and drop 'const' modifier from types"; + let description = [{ + This pass checks assignments of 'const' values, ensuring that 'const' + assignment is not dependent on non-'const' when conditions. + + Once 'const' usage is checked, all 'const' types are replaced with their + non-'const' versions, simplifying downstream passes so that they do not + need to take 'const' into account. + }]; + let constructor = "circt::firrtl::createDropConstPass()"; +} + def FinalizeIR : Pass<"firrtl-finalize-ir", "mlir::ModuleOp"> { let summary = "Perform final IR mutations after ExportVerilog"; let description = [{ diff --git a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp index 9490827d915e..3bb6e093beef 100644 --- a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp @@ -582,6 +582,19 @@ FIRRTLBaseType FIRRTLBaseType::getConstType(bool isConst) { }); } +/// Return this type with a 'const' modifiers dropped +FIRRTLBaseType FIRRTLBaseType::getAllConstDroppedType() { + return TypeSwitch(*this) + .Case([&](auto type) { return type.getConstType(false); }) + .Case( + [&](auto type) { return type.getAllConstDroppedType(); }) + .Default([](Type) { + llvm_unreachable("unknown FIRRTL type"); + return FIRRTLBaseType(); + }); +} + /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType FIRRTLBaseType::getMaskType() { @@ -1220,6 +1233,18 @@ BundleType BundleType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } +BundleType BundleType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + + SmallVector constDroppedElements( + llvm::map_range(getElements(), [](BundleElement element) { + element.type = element.type.getAllConstDroppedType(); + return element; + })); + return get(getContext(), constDroppedElements, false); +} + std::optional BundleType::getElementIndex(StringAttr name) { for (const auto &it : llvm::enumerate(getElements())) { auto element = it.value(); @@ -1614,6 +1639,13 @@ FVectorType FVectorType::getConstType(bool isConst) { return get(getElementType(), getNumElements(), isConst); } +FVectorType FVectorType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + return get(getElementType().getAllConstDroppedType(), getNumElements(), + false); +} + uint64_t FVectorType::getFieldID(uint64_t index) { return 1 + index * (getElementType().getMaxFieldID() + 1); } @@ -1845,6 +1877,18 @@ FEnumType FEnumType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } +FEnumType FEnumType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + + SmallVector constDroppedElements( + llvm::map_range(getElements(), [](EnumElement element) { + element.type = element.type.getAllConstDroppedType(); + return element; + })); + return get(getContext(), constDroppedElements, false); +} + /// Return a pair with the 'isPassive' and 'containsAnalog' bits. RecursiveTypeProperties FEnumType::getRecursiveTypeProperties() const { return getImpl()->recProps; diff --git a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt index a867dec06ddf..6fa681beab4e 100755 --- a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt +++ b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt @@ -3,6 +3,7 @@ add_circt_dialect_library(CIRCTFIRRTLTransforms BlackBoxReader.cpp CheckCombCycles.cpp CheckCombLoops.cpp + DropConst.cpp CreateSiFiveMetadata.cpp Dedup.cpp DropName.cpp diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp new file mode 100644 index 000000000000..3f2c50d6e732 --- /dev/null +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -0,0 +1,184 @@ +//===- DropConst.cpp - Check and remove const types -------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines the DropConst pass. +// +//===----------------------------------------------------------------------===// + +#include "PassDetails.h" +#include "circt/Dialect/FIRRTL/FIRRTLOps.h" +#include "circt/Dialect/FIRRTL/FIRRTLTypes.h" +#include "circt/Dialect/FIRRTL/FIRRTLUtils.h" +#include "circt/Dialect/FIRRTL/FIRRTLVisitors.h" +#include "circt/Dialect/FIRRTL/Passes.h" + +#define DEBUG_TYPE "drop-const" + +using namespace circt; +using namespace firrtl; + +namespace { + +class OpVisitor : public FIRRTLVisitor { +public: + using FIRRTLVisitor::visitExpr; + using FIRRTLVisitor::visitDecl; + using FIRRTLVisitor::visitStmt; + + LogicalResult checkModule(FModuleLike module) { + auto fmodule = dyn_cast(*module); + // Find 'const' ports + auto portTypes = SmallVector(module.getPortTypes()); + for (size_t portIndex = 0, numPorts = module.getPortTypes().size(); + portIndex < numPorts; ++portIndex) { + if (auto convertedType = convertType(module.getPortType(portIndex))) { + // If this is an FModuleOp, register the block argument to drop 'const' + if (fmodule) + constValuesToConvert.push_back( + {fmodule.getArgument(portIndex), convertedType}); + portTypes[portIndex] = TypeAttr::get(convertedType); + } + } + + // Update the module signature with non-'const' ports + module->setAttr(FModuleLike::getPortTypesAttrName(), + ArrayAttr::get(module.getContext(), portTypes)); + + if (!fmodule) + return result; + + // Check the module body + visitDecl(fmodule); + + // Drop 'const' from all registered values + for (auto [value, type] : constValuesToConvert) + value.setType(type); + + return result; + } + + void visitDecl(FModuleOp module) { + for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) + dispatchVisitor(&op); + } + + void visitExpr(ConstCastOp constCast) { + // Remove any `ConstCastOp`, replacing results with inputs + constCast.getResult().replaceAllUsesWith(constCast.getInput()); + constCast->erase(); + } + + void visitStmt(WhenOp when) { + // Check that 'const' destinations are only connected to within + // 'const'-conditioned when blocks, unless the destination is local to the + // when block's scope + + auto *previousNonConstConditionedBlock = nonConstConditionedBlock; + bool isWithinNonconstCondition = !when.getCondition().getType().isConst(); + + if (isWithinNonconstCondition) + nonConstConditionedBlock = &when.getThenBlock(); + for (auto &op : llvm::make_early_inc_range(when.getThenBlock())) + dispatchVisitor(&op); + + if (when.hasElseRegion()) { + if (isWithinNonconstCondition) + nonConstConditionedBlock = &when.getElseBlock(); + for (auto &op : llvm::make_early_inc_range(when.getElseBlock())) + dispatchVisitor(&op); + } + + nonConstConditionedBlock = previousNonConstConditionedBlock; + } + + void visitStmt(ConnectOp connect) { handleConnect(connect); } + + void visitStmt(StrictConnectOp connect) { handleConnect(connect); } + + void visitUnhandledOp(Operation *op) { + // Register any 'const' results to drop 'const' + for (auto [index, result] : llvm::enumerate(op->getResults())) { + if (auto convertedType = convertType(result.getType())) + constValuesToConvert.push_back({result, convertedType}); + } + } + +private: + /// Returns null type if no conversion is needed + Type convertType(Type type) { + if (auto base = type.dyn_cast()) { + return convertType(base); + } + + if (auto refType = type.dyn_cast()) { + if (auto converted = convertType(refType.getType())) + return RefType::get(converted.cast(), + refType.getForceable()); + } + + return {}; + } + + /// Returns null type if no conversion is needed + FIRRTLBaseType convertType(FIRRTLBaseType type) { + auto nonConstType = type.getAllConstDroppedType(); + return nonConstType != type ? nonConstType : FIRRTLBaseType{}; + } + + void handleConnect(FConnectLike connect) { + auto dest = connect.getDest(); + auto destType = dest.getType(); + if (nonConstConditionedBlock && containsConst(destType)) { + // 'const' connects are allowed if `dest` is local to the non-'const' when + // block + auto *destBlock = dest.getParentBlock(); + auto *block = connect->getBlock(); + while (block) { + // The connect is local to the `dest` declaration, both local to the + // non-'const' block, so the connect is valid + if (block == destBlock) + return; + // The connect is within the non-'const' condition, non-local to the + // `dest` declaration, so the connect is invalid + if (block == nonConstConditionedBlock) + break; + + if (auto *parentOp = block->getParentOp()) + block = parentOp->getBlock(); + else + break; + } + + result = failure(); + if (isConst(destType)) + connect.emitOpError() << "assignment to 'const' type " << destType + << " is dependent on a non-'const' condition"; + else + connect->emitOpError() + << "assignment to nested 'const' member of type " << destType + << " is dependent on a non-'const' condition"; + } + } + + SmallVector> constValuesToConvert; + Block *nonConstConditionedBlock = nullptr; + LogicalResult result = success(); +}; + +class DropConstPass : public DropConstBase { + void runOnOperation() override { + OpVisitor visitor; + if (failed(visitor.checkModule(getOperation()))) + signalPassFailure(); + } +}; +} // namespace + +std::unique_ptr circt::firrtl::createDropConstPass() { + return std::make_unique(); +} diff --git a/lib/Firtool/Firtool.cpp b/lib/Firtool/Firtool.cpp index 3dbbbe262339..dd02e6aa71a2 100644 --- a/lib/Firtool/Firtool.cpp +++ b/lib/Firtool/Firtool.cpp @@ -60,6 +60,8 @@ LogicalResult firtool::populateCHIRRTLToLowFIRRTL(mlir::PassManager &pm, } } + pm.nest().nestAny().addPass(firrtl::createDropConstPass()); + if (opt.dedup) pm.nest().addPass(firrtl::createDedupPass()); diff --git a/test/Dialect/FIRRTL/drop-const-errors.mlir b/test/Dialect/FIRRTL/drop-const-errors.mlir new file mode 100644 index 000000000000..91ab06940745 --- /dev/null +++ b/test/Dialect/FIRRTL/drop-const-errors.mlir @@ -0,0 +1,40 @@ +// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(any(firrtl-drop-const)))' -verify-diagnostics --split-input-file %s + +firrtl.circuit "CheckConstAssignInNonConstCondition" { +firrtl.module @CheckConstAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } else { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + +firrtl.circuit "CheckNestedConstAssignInNonConstCondition" { +firrtl.module @CheckNestedConstAssignInNonConstCondition(in %constP: !firrtl.const.uint<1>, in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.when %constP : !firrtl.const.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } + } +} +} + +// ----- + +firrtl.circuit "CheckBundleConstFieldAssignInNonConstCondition" { +firrtl.module @CheckBundleConstFieldAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> + } else { + // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in :!firrtl.bundle>, !firrtl.bundle> + } +} +} diff --git a/test/Dialect/FIRRTL/drop-const.mlir b/test/Dialect/FIRRTL/drop-const.mlir new file mode 100644 index 000000000000..a5df2e3653ba --- /dev/null +++ b/test/Dialect/FIRRTL/drop-const.mlir @@ -0,0 +1,138 @@ +// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(any(firrtl-drop-const)))' %s | FileCheck %s --implicit-check-not=const. +firrtl.circuit "DropConst" { +firrtl.module @DropConst() {} + +// Const is dropped from extmodule signature +// CHECK-LABEL: firrtl.extmodule @ConstPortExtModule( +// CHECK-SAME: in a: !firrtl.uint<1> +// CHECK-SAME: in b: !firrtl.bundle> +// CHECK-SAME: in c: !firrtl.bundle>, +// CHECK-SAME: in d: !firrtl.vector, 3>, +// CHECK-SAME: in e: !firrtl.vector, 3>, +// CHECK-SAME: in f: !firrtl.enum, b: uint<1>>, +// CHECK-SAME: in g: !firrtl.enum, b: uint<1>>, +// CHECK-SAME: out h: !firrtl.probe>) +firrtl.extmodule @ConstPortExtModule( + in a: !firrtl.const.uint<1>, + in b: !firrtl.const.bundle>, + in c: !firrtl.bundle>, + in d: !firrtl.const.vector, 3>, + in e: !firrtl.vector, 3>, + in f: !firrtl.const.enum, b: uint<1>>, + in g: !firrtl.enum, b: const.uint<1>>, + out h: !firrtl.probe> +) + +// Const is dropped from module signature and ops +// CHECK-LABEL: firrtl.module @ConstPortModule( +// CHECK-SAME: in %a: !firrtl.uint<1> +// CHECK-SAME: in %b: !firrtl.bundle> +// CHECK-SAME: in %c: !firrtl.bundle>, +// CHECK-SAME: in %d: !firrtl.vector, 3>, +// CHECK-SAME: in %e: !firrtl.vector, 3>, +// CHECK-SAME: in %f: !firrtl.enum, b: uint<1>>, +// CHECK-SAME: in %g: !firrtl.enum, b: uint<1>>, +// CHECK-SAME: out %h: !firrtl.probe>) +firrtl.module @ConstPortModule( + in %a: !firrtl.const.uint<1>, + in %b: !firrtl.const.bundle>, + in %c: !firrtl.bundle>, + in %d: !firrtl.const.vector, 3>, + in %e: !firrtl.vector, 3>, + in %f: !firrtl.const.enum, b: uint<1>>, + in %g: !firrtl.enum, b: const.uint<1>>, + out %h: !firrtl.probe> +) { + // CHECK-NEXT: firrtl.instance inst @ConstPortExtModule( + // CHECK-SAME: in a: !firrtl.uint<1> + // CHECK-SAME: in b: !firrtl.bundle> + // CHECK-SAME: in c: !firrtl.bundle>, + // CHECK-SAME: in d: !firrtl.vector, 3>, + // CHECK-SAME: in e: !firrtl.vector, 3>, + // CHECK-SAME: in f: !firrtl.enum, b: uint<1>>, + // CHECK-SAME: in g: !firrtl.enum, b: uint<1>>, + // CHECK-SAME: out h: !firrtl.probe>) + %a2, %b2, %c2, %d2, %e2, %f2, %g2, %h2 = firrtl.instance inst @ConstPortExtModule( + in a: !firrtl.const.uint<1>, + in b: !firrtl.const.bundle>, + in c: !firrtl.bundle>, + in d: !firrtl.const.vector, 3>, + in e: !firrtl.vector, 3>, + in f: !firrtl.const.enum, b: uint<1>>, + in g: !firrtl.enum, b: const.uint<1>>, + out h: !firrtl.probe> + ) + + firrtl.strictconnect %a2, %a : !firrtl.const.uint<1> + firrtl.strictconnect %b2, %b : !firrtl.const.bundle> + firrtl.strictconnect %c2, %c : !firrtl.bundle> + firrtl.strictconnect %d2, %d : !firrtl.const.vector, 3> + firrtl.strictconnect %e2, %e : !firrtl.vector, 3> + firrtl.strictconnect %f2, %f : !firrtl.const.enum, b: uint<1>> + firrtl.strictconnect %g2, %g : !firrtl.enum, b: const.uint<1>> + firrtl.ref.define %h, %h2 : !firrtl.probe> +} + +// Const-cast ops are erased +// CHECK-LABEL: firrtl.module @ConstCastErase +firrtl.module @ConstCastErase(in %in: !firrtl.const.uint<1>, out %out: !firrtl.uint<1>) { + // CHECK-NOT: firrtl.constCast + // CHECK-NEXT: firrtl.strictconnect %out, %in : !firrtl.uint<1> + %0 = firrtl.constCast %in : (!firrtl.const.uint<1>) -> !firrtl.uint<1> + firrtl.strictconnect %out, %0 : !firrtl.uint<1> +} + +// Const connections can occur within const-conditioned whens +// CHECK-LABEL: firrtl.module @ConstConditionConstAssign +firrtl.module @ConstConditionConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.const.sint<2>, in %in2: !firrtl.const.sint<2>, out %out: !firrtl.const.sint<2>) { + // CHECK: firrtl.when %cond : !firrtl.uint<1> + firrtl.when %cond : !firrtl.const.uint<1> { + // CHECK: firrtl.strictconnect %out, %in1 : !firrtl.sint<2> + firrtl.strictconnect %out, %in1 : !firrtl.const.sint<2> + } else { + // CHECK: firrtl.strictconnect %out, %in2 : !firrtl.sint<2> + firrtl.strictconnect %out, %in2 : !firrtl.const.sint<2> + } +} + +// Non-const connections can occur within const-conditioned whens +// CHECK-LABEL: firrtl.module @ConstConditionNonConstAssign +firrtl.module @ConstConditionNonConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.sint<2>, in %in2: !firrtl.sint<2>, out %out: !firrtl.sint<2>) { + // CHECK: firrtl.when %cond : !firrtl.uint<1> + firrtl.when %cond : !firrtl.const.uint<1> { + firrtl.strictconnect %out, %in1 : !firrtl.sint<2> + } else { + firrtl.strictconnect %out, %in2 : !firrtl.sint<2> + } +} + +// Const connections can occur when the destination is local to a non-const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstAssign +firrtl.module @NonConstWhenLocalConstAssign(in %cond: !firrtl.uint<1>) { + firrtl.when %cond : !firrtl.uint<1> { + // CHECK: firrtl.wire : !firrtl.uint<9> + // CHECK-NEXT: firrtl.constant 0 : !firrtl.uint<9> + %w = firrtl.wire : !firrtl.const.uint<9> + %c = firrtl.constant 0 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } +} + +// Const connections can occur when the destination is local to a non-const +// conditioned when block and the connection is inside a const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign +firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign(in %cond: !firrtl.uint<1>, in %constCond: !firrtl.const.uint<1>) { + firrtl.when %cond : !firrtl.uint<1> { + // CHECK: firrtl.wire : !firrtl.uint<9> + %w = firrtl.wire : !firrtl.const.uint<9> + // CHECK-NEXT: firrtl.when %constCond : !firrtl.uint<1> + firrtl.when %constCond : !firrtl.const.uint<1> { + %c = firrtl.constant 0 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } else { + %c = firrtl.constant 1 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } + } +} +} \ No newline at end of file From 27ca3f127e577ba04b31bdfdcc72f528eebc6b7f Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Wed, 17 May 2023 13:05:07 -0600 Subject: [PATCH 02/11] PR updates --- lib/Dialect/FIRRTL/Transforms/CMakeLists.txt | 2 +- lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 117 ++++++++++++------- test/Dialect/FIRRTL/drop-const-errors.mlir | 12 +- test/Dialect/FIRRTL/drop-const.mlir | 12 ++ 4 files changed, 99 insertions(+), 44 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt index 6fa681beab4e..787577d73cd6 100755 --- a/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt +++ b/lib/Dialect/FIRRTL/Transforms/CMakeLists.txt @@ -3,9 +3,9 @@ add_circt_dialect_library(CIRCTFIRRTLTransforms BlackBoxReader.cpp CheckCombCycles.cpp CheckCombLoops.cpp - DropConst.cpp CreateSiFiveMetadata.cpp Dedup.cpp + DropConst.cpp DropName.cpp EmitOMIR.cpp ExpandWhens.cpp diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index 3f2c50d6e732..a392ac52aa5d 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -16,21 +16,42 @@ #include "circt/Dialect/FIRRTL/FIRRTLUtils.h" #include "circt/Dialect/FIRRTL/FIRRTLVisitors.h" #include "circt/Dialect/FIRRTL/Passes.h" +#include "llvm/Support/SaveAndRestore.h" #define DEBUG_TYPE "drop-const" using namespace circt; using namespace firrtl; -namespace { +// NOLINTBEGIN(misc-no-recursion) +static bool typeHasConstLeaf(FIRRTLBaseType type, bool outerTypeIsConst = false, + bool isFlip = false) { + auto typeIsConst = outerTypeIsConst || type.isConst(); + + if (typeIsConst && type.isPassive()) + return !isFlip; + + if (auto bundleType = type.dyn_cast()) + return llvm::any_of(bundleType.getElements(), [&](auto &element) { + return typeHasConstLeaf(element.type, typeIsConst, + isFlip ^ element.isFlip); + }); + + if (auto vectorType = type.dyn_cast()) + return typeHasConstLeaf(vectorType.getElementType(), typeIsConst, isFlip); -class OpVisitor : public FIRRTLVisitor { + return typeIsConst && !isFlip; +} +// NOLINTEND(misc-no-recursion) + +namespace { +class OpVisitor : public FIRRTLVisitor { public: - using FIRRTLVisitor::visitExpr; - using FIRRTLVisitor::visitDecl; - using FIRRTLVisitor::visitStmt; + using FIRRTLVisitor::visitExpr; + using FIRRTLVisitor::visitDecl; + using FIRRTLVisitor::visitStmt; - LogicalResult checkModule(FModuleLike module) { + LogicalResult handleModule(FModuleLike module) { auto fmodule = dyn_cast(*module); // Find 'const' ports auto portTypes = SmallVector(module.getPortTypes()); @@ -50,66 +71,79 @@ class OpVisitor : public FIRRTLVisitor { ArrayAttr::get(module.getContext(), portTypes)); if (!fmodule) - return result; + return success(); // Check the module body - visitDecl(fmodule); + if (failed(visitDecl(fmodule))) + return failure(); // Drop 'const' from all registered values for (auto [value, type] : constValuesToConvert) value.setType(type); - return result; + return success(); } - void visitDecl(FModuleOp module) { - for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) - dispatchVisitor(&op); + LogicalResult visitDecl(FModuleOp module) { + for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) { + if (failed(dispatchVisitor(&op))) + return failure(); + } + return success(); } - void visitExpr(ConstCastOp constCast) { + LogicalResult visitExpr(ConstCastOp constCast) { // Remove any `ConstCastOp`, replacing results with inputs constCast.getResult().replaceAllUsesWith(constCast.getInput()); constCast->erase(); + return success(); } - void visitStmt(WhenOp when) { + LogicalResult visitStmt(WhenOp when) { // Check that 'const' destinations are only connected to within // 'const'-conditioned when blocks, unless the destination is local to the // when block's scope - auto *previousNonConstConditionedBlock = nonConstConditionedBlock; + llvm::SaveAndRestore blockSave(nonConstConditionedBlock); bool isWithinNonconstCondition = !when.getCondition().getType().isConst(); if (isWithinNonconstCondition) nonConstConditionedBlock = &when.getThenBlock(); for (auto &op : llvm::make_early_inc_range(when.getThenBlock())) - dispatchVisitor(&op); + if (failed(dispatchVisitor(&op))) + return failure(); if (when.hasElseRegion()) { if (isWithinNonconstCondition) nonConstConditionedBlock = &when.getElseBlock(); for (auto &op : llvm::make_early_inc_range(when.getElseBlock())) - dispatchVisitor(&op); + if (failed(dispatchVisitor(&op))) + return failure(); } - nonConstConditionedBlock = previousNonConstConditionedBlock; + return success(); } - void visitStmt(ConnectOp connect) { handleConnect(connect); } + LogicalResult visitStmt(ConnectOp connect) { return handleConnect(connect); } - void visitStmt(StrictConnectOp connect) { handleConnect(connect); } + LogicalResult visitStmt(StrictConnectOp connect) { + return handleConnect(connect); + } - void visitUnhandledOp(Operation *op) { + LogicalResult visitUnhandledOp(Operation *op) { // Register any 'const' results to drop 'const' for (auto [index, result] : llvm::enumerate(op->getResults())) { if (auto convertedType = convertType(result.getType())) constValuesToConvert.push_back({result, convertedType}); } + + return success(); } + LogicalResult visitInvalidOp(Operation *op) { return success(); } + private: - /// Returns null type if no conversion is needed + /// Returns null type if no conversion is needed. Type convertType(Type type) { if (auto base = type.dyn_cast()) { return convertType(base); @@ -124,25 +158,25 @@ class OpVisitor : public FIRRTLVisitor { return {}; } - /// Returns null type if no conversion is needed + /// Returns null type if no conversion is needed. FIRRTLBaseType convertType(FIRRTLBaseType type) { auto nonConstType = type.getAllConstDroppedType(); return nonConstType != type ? nonConstType : FIRRTLBaseType{}; } - void handleConnect(FConnectLike connect) { + LogicalResult handleConnect(FConnectLike connect) { auto dest = connect.getDest(); - auto destType = dest.getType(); - if (nonConstConditionedBlock && containsConst(destType)) { + auto destType = dest.getType().cast(); + if (nonConstConditionedBlock && destType && destType.containsConst()) { // 'const' connects are allowed if `dest` is local to the non-'const' when - // block + // block. auto *destBlock = dest.getParentBlock(); auto *block = connect->getBlock(); while (block) { // The connect is local to the `dest` declaration, both local to the - // non-'const' block, so the connect is valid + // non-'const' block, so the connect is valid. if (block == destBlock) - return; + return success(); // The connect is within the non-'const' condition, non-local to the // `dest` declaration, so the connect is invalid if (block == nonConstConditionedBlock) @@ -154,26 +188,29 @@ class OpVisitor : public FIRRTLVisitor { break; } - result = failure(); - if (isConst(destType)) - connect.emitOpError() << "assignment to 'const' type " << destType - << " is dependent on a non-'const' condition"; - else - connect->emitOpError() - << "assignment to nested 'const' member of type " << destType - << " is dependent on a non-'const' condition"; + // A const dest is allowed if leaf elements are effectively non-const due + // to flips. + if (typeHasConstLeaf(destType)) { + if (destType.isConst()) + return connect.emitOpError() + << "assignment to 'const' type " << destType + << " is dependent on a non-'const' condition"; + return connect->emitOpError() + << "assignment to nested 'const' member of type " << destType + << " is dependent on a non-'const' condition"; + } } + + return success(); } SmallVector> constValuesToConvert; Block *nonConstConditionedBlock = nullptr; - LogicalResult result = success(); }; class DropConstPass : public DropConstBase { void runOnOperation() override { - OpVisitor visitor; - if (failed(visitor.checkModule(getOperation()))) + if (failed(OpVisitor().handleModule(getOperation()))) signalPassFailure(); } }; diff --git a/test/Dialect/FIRRTL/drop-const-errors.mlir b/test/Dialect/FIRRTL/drop-const-errors.mlir index 91ab06940745..161d1746e5d7 100644 --- a/test/Dialect/FIRRTL/drop-const-errors.mlir +++ b/test/Dialect/FIRRTL/drop-const-errors.mlir @@ -5,6 +5,15 @@ firrtl.module @CheckConstAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %i firrtl.when %p : !firrtl.uint<1> { // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + +firrtl.circuit "CheckConstAssignInNonConstConditionElse" { +firrtl.module @CheckConstAssignInNonConstConditionElse(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { } else { // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> @@ -32,9 +41,6 @@ firrtl.module @CheckBundleConstFieldAssignInNonConstCondition(in %p: !firrtl.uin firrtl.when %p : !firrtl.uint<1> { // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> - } else { - // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in :!firrtl.bundle>, !firrtl.bundle> } } } diff --git a/test/Dialect/FIRRTL/drop-const.mlir b/test/Dialect/FIRRTL/drop-const.mlir index a5df2e3653ba..dd5bad45793a 100644 --- a/test/Dialect/FIRRTL/drop-const.mlir +++ b/test/Dialect/FIRRTL/drop-const.mlir @@ -135,4 +135,16 @@ firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign(in %cond: !firrtl.uin } } } + +firrtl.module @NonConstWhenConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.const.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.connect %out, %in : !firrtl.const.bundle>, !firrtl.bundle> + } +} + +firrtl.module @NonConstWhenNestedConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> + } +} } \ No newline at end of file From 6bab4f94f03e26a804362afebffa18d8356d5e65 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Wed, 17 May 2023 16:39:31 -0600 Subject: [PATCH 03/11] Set types to non-const as they are encountered (backwards) --- lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 33 ++++++++++----------- test/Dialect/FIRRTL/drop-const-errors.mlir | 14 +++++++++ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index a392ac52aa5d..9562bb9da734 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -53,6 +53,13 @@ class OpVisitor : public FIRRTLVisitor { LogicalResult handleModule(FModuleLike module) { auto fmodule = dyn_cast(*module); + + // Check the module body + if (fmodule) { + if (failed(visitDecl(fmodule))) + return failure(); + } + // Find 'const' ports auto portTypes = SmallVector(module.getPortTypes()); for (size_t portIndex = 0, numPorts = module.getPortTypes().size(); @@ -60,8 +67,7 @@ class OpVisitor : public FIRRTLVisitor { if (auto convertedType = convertType(module.getPortType(portIndex))) { // If this is an FModuleOp, register the block argument to drop 'const' if (fmodule) - constValuesToConvert.push_back( - {fmodule.getArgument(portIndex), convertedType}); + fmodule.getArgument(portIndex).setType(convertedType); portTypes[portIndex] = TypeAttr::get(convertedType); } } @@ -70,22 +76,12 @@ class OpVisitor : public FIRRTLVisitor { module->setAttr(FModuleLike::getPortTypesAttrName(), ArrayAttr::get(module.getContext(), portTypes)); - if (!fmodule) - return success(); - - // Check the module body - if (failed(visitDecl(fmodule))) - return failure(); - - // Drop 'const' from all registered values - for (auto [value, type] : constValuesToConvert) - value.setType(type); - return success(); } LogicalResult visitDecl(FModuleOp module) { - for (auto &op : llvm::make_early_inc_range(*module.getBodyBlock())) { + for (auto &op : + llvm::make_early_inc_range(llvm::reverse(*module.getBodyBlock()))) { if (failed(dispatchVisitor(&op))) return failure(); } @@ -109,14 +105,16 @@ class OpVisitor : public FIRRTLVisitor { if (isWithinNonconstCondition) nonConstConditionedBlock = &when.getThenBlock(); - for (auto &op : llvm::make_early_inc_range(when.getThenBlock())) + for (auto &op : + llvm::make_early_inc_range(llvm::reverse(when.getThenBlock()))) if (failed(dispatchVisitor(&op))) return failure(); if (when.hasElseRegion()) { if (isWithinNonconstCondition) nonConstConditionedBlock = &when.getElseBlock(); - for (auto &op : llvm::make_early_inc_range(when.getElseBlock())) + for (auto &op : + llvm::make_early_inc_range(llvm::reverse(when.getElseBlock()))) if (failed(dispatchVisitor(&op))) return failure(); } @@ -134,7 +132,7 @@ class OpVisitor : public FIRRTLVisitor { // Register any 'const' results to drop 'const' for (auto [index, result] : llvm::enumerate(op->getResults())) { if (auto convertedType = convertType(result.getType())) - constValuesToConvert.push_back({result, convertedType}); + result.setType(convertedType); } return success(); @@ -204,7 +202,6 @@ class OpVisitor : public FIRRTLVisitor { return success(); } - SmallVector> constValuesToConvert; Block *nonConstConditionedBlock = nullptr; }; diff --git a/test/Dialect/FIRRTL/drop-const-errors.mlir b/test/Dialect/FIRRTL/drop-const-errors.mlir index 161d1746e5d7..549f347e3070 100644 --- a/test/Dialect/FIRRTL/drop-const-errors.mlir +++ b/test/Dialect/FIRRTL/drop-const-errors.mlir @@ -23,6 +23,20 @@ firrtl.module @CheckConstAssignInNonConstConditionElse(in %p: !firrtl.uint<1>, i // ----- +// This tests that values aren't being set to non-const before their usage is checked. +firrtl.circuit "CheckIntermediateValueConstAssignInNonConstCondition" { +firrtl.extmodule @Inner(in a : !firrtl.const.uint<2>) +firrtl.module @CheckIntermediateValueConstAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + %a = firrtl.instance inner @Inner(in a : !firrtl.const.uint<2>) + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %a, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + firrtl.circuit "CheckNestedConstAssignInNonConstCondition" { firrtl.module @CheckNestedConstAssignInNonConstCondition(in %constP: !firrtl.const.uint<1>, in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { firrtl.when %p : !firrtl.uint<1> { From 739a89236a4d2fa3cd3a47746c9202062243da5b Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Thu, 18 May 2023 11:25:04 -0600 Subject: [PATCH 04/11] Fix cast --- lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index 9562bb9da734..a85c69d94d25 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -164,7 +164,7 @@ class OpVisitor : public FIRRTLVisitor { LogicalResult handleConnect(FConnectLike connect) { auto dest = connect.getDest(); - auto destType = dest.getType().cast(); + auto destType = dest.getType().dyn_cast(); if (nonConstConditionedBlock && destType && destType.containsConst()) { // 'const' connects are allowed if `dest` is local to the non-'const' when // block. From 0cbc7351b5975f2e1bda3f43642579243ba09028 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Thu, 18 May 2023 12:42:37 -0600 Subject: [PATCH 05/11] Move conditional assignment checking to connect verifiers --- include/circt/Dialect/FIRRTL/Passes.td | 13 +- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 90 +++++++++ lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 193 ++++---------------- test/Dialect/FIRRTL/connect-errors.mlir | 84 +++++++++ test/Dialect/FIRRTL/connect.mlir | 80 ++++++++ test/Dialect/FIRRTL/drop-const-errors.mlir | 60 ------ test/Dialect/FIRRTL/drop-const.mlir | 59 +----- 7 files changed, 296 insertions(+), 283 deletions(-) delete mode 100644 test/Dialect/FIRRTL/drop-const-errors.mlir diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 2b0a62603b26..5eba71eec21a 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -692,14 +692,13 @@ def VBToBV : Pass<"firrtl-vb-to-bv", "firrtl::CircuitOp"> { } def DropConst : InterfacePass<"firrtl-drop-const", "firrtl::FModuleLike"> { - let summary = "Check usage of and drop 'const' modifier from types"; + let summary = "Drop 'const' modifier from types"; let description = [{ - This pass checks assignments of 'const' values, ensuring that 'const' - assignment is not dependent on non-'const' when conditions. - - Once 'const' usage is checked, all 'const' types are replaced with their - non-'const' versions, simplifying downstream passes so that they do not - need to take 'const' into account. + This pass drops the 'const' modifier from all types and removes all + const-cast ops. + + This simplifies downstream passes and folds so that they do not need to + take 'const' into account. }]; let constructor = "circt::firrtl::createDropConstPass()"; } diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index b6b57c1d622e..fdd6d3fc36cc 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -2331,6 +2331,90 @@ static LogicalResult checkConnectFlow(Operation *connect) { return success(); } +namespace { +enum class ConstLeafKind : uint8_t { None = 0, Normal = 1 << 0, Flip = 1 << 1 }; +} // end anonymous namespace + +// NOLINTBEGIN(misc-no-recursion) +/// Checks if the type has any 'const' leaf elements that are normal or flip +static ConstLeafKind typeConstLeafKinds(FIRRTLBaseType type, + bool outerTypeIsConst = false, + bool isFlip = false) { + auto typeIsConst = outerTypeIsConst || type.isConst(); + + if (typeIsConst && type.isPassive()) + return isFlip ? ConstLeafKind::Flip : ConstLeafKind::Normal; + + if (auto bundleType = type.dyn_cast()) { + auto kinds = ConstLeafKind::None; + for (auto &element : bundleType.getElements()) { + auto elementKinds = typeConstLeafKinds(element.type, typeIsConst, + isFlip ^ element.isFlip); + kinds = static_cast(static_cast(kinds) | + static_cast(elementKinds)); + } + return kinds; + } + + if (auto vectorType = type.dyn_cast()) + return typeConstLeafKinds(vectorType.getElementType(), typeIsConst, isFlip); + + if (typeIsConst) + return isFlip ? ConstLeafKind::Normal : ConstLeafKind::Flip; + return ConstLeafKind::None; +} +// NOLINTEND(misc-no-recursion) + +/// Checks that connections to 'const' destinations are not dependent on +/// non-'const' conditions in when blocks. +static LogicalResult checkConnectConditionality(FConnectLike connect) { + auto dest = connect.getDest(); + auto destType = dest.getType().dyn_cast(); + auto src = connect.getSrc(); + auto srcType = src.getType().dyn_cast(); + if (!destType || !srcType) + return success(); + + auto checkConstConditionality = [&](Value value, + FIRRTLBaseType type) -> LogicalResult { + auto *definingBlock = value.getParentBlock(); + auto *block = connect->getBlock(); + while (block && block != definingBlock) { + auto *parentOp = block->getParentOp(); + + if (auto whenOp = dyn_cast(parentOp); + whenOp && !whenOp.getCondition().getType().isConst()) { + if (type.isConst()) + return connect.emitOpError() + << "assignment to 'const' type " << type + << " is dependent on a non-'const' condition"; + return connect->emitOpError() + << "assignment to nested 'const' member of type " << type + << " is dependent on a non-'const' condition"; + } + + block = parentOp->getBlock(); + } + return success(); + }; + + // Check destination if it contains 'const' leaves + if (destType.containsConst() && + static_cast(typeConstLeafKinds(destType)) & + static_cast(ConstLeafKind::Normal) && + failed(checkConstConditionality(dest, destType))) + return failure(); + + // Check source if it contains 'const' 'flip' leaves + if (srcType.containsConst() && + static_cast(typeConstLeafKinds(srcType)) & + static_cast(ConstLeafKind::Flip) && + failed(checkConstConditionality(src, srcType))) + return failure(); + + return success(); +} + LogicalResult ConnectOp::verify() { auto dstType = getDest().getType(); auto srcType = getSrc().getType(); @@ -2360,6 +2444,9 @@ LogicalResult ConnectOp::verify() { if (failed(checkConnectFlow(*this))) return failure(); + if (failed(checkConnectConditionality(*this))) + return failure(); + return success(); } @@ -2376,6 +2463,9 @@ LogicalResult StrictConnectOp::verify() { if (failed(checkConnectFlow(*this))) return failure(); + if (failed(checkConnectConditionality(*this))) + return failure(); + return success(); } diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index a85c69d94d25..efea651f4d94 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -23,41 +23,48 @@ using namespace circt; using namespace firrtl; -// NOLINTBEGIN(misc-no-recursion) -static bool typeHasConstLeaf(FIRRTLBaseType type, bool outerTypeIsConst = false, - bool isFlip = false) { - auto typeIsConst = outerTypeIsConst || type.isConst(); - - if (typeIsConst && type.isPassive()) - return !isFlip; +/// Returns null type if no conversion is needed. +static FIRRTLBaseType convertType(FIRRTLBaseType type) { + auto nonConstType = type.getAllConstDroppedType(); + return nonConstType != type ? nonConstType : FIRRTLBaseType{}; +} - if (auto bundleType = type.dyn_cast()) - return llvm::any_of(bundleType.getElements(), [&](auto &element) { - return typeHasConstLeaf(element.type, typeIsConst, - isFlip ^ element.isFlip); - }); +/// Returns null type if no conversion is needed. +static Type convertType(Type type) { + if (auto base = type.dyn_cast()) { + return convertType(base); + } - if (auto vectorType = type.dyn_cast()) - return typeHasConstLeaf(vectorType.getElementType(), typeIsConst, isFlip); + if (auto refType = type.dyn_cast()) { + if (auto converted = convertType(refType.getType())) + return RefType::get(converted.cast(), + refType.getForceable()); + } - return typeIsConst && !isFlip; + return {}; } -// NOLINTEND(misc-no-recursion) namespace { -class OpVisitor : public FIRRTLVisitor { -public: - using FIRRTLVisitor::visitExpr; - using FIRRTLVisitor::visitDecl; - using FIRRTLVisitor::visitStmt; - - LogicalResult handleModule(FModuleLike module) { +class DropConstPass : public DropConstBase { + void runOnOperation() override { + auto module = getOperation(); auto fmodule = dyn_cast(*module); - // Check the module body + // Check the module body if present if (fmodule) { - if (failed(visitDecl(fmodule))) - return failure(); + fmodule->walk([](Operation *op) { + if (auto constCastOp = dyn_cast(op)) { + // Remove any `ConstCastOp`, replacing results with inputs + constCastOp.getResult().replaceAllUsesWith(constCastOp.getInput()); + constCastOp->erase(); + return; + } + + for (auto result : op->getResults()) { + if (auto convertedType = convertType(result.getType())) + result.setType(convertedType); + } + }); } // Find 'const' ports @@ -75,140 +82,6 @@ class OpVisitor : public FIRRTLVisitor { // Update the module signature with non-'const' ports module->setAttr(FModuleLike::getPortTypesAttrName(), ArrayAttr::get(module.getContext(), portTypes)); - - return success(); - } - - LogicalResult visitDecl(FModuleOp module) { - for (auto &op : - llvm::make_early_inc_range(llvm::reverse(*module.getBodyBlock()))) { - if (failed(dispatchVisitor(&op))) - return failure(); - } - return success(); - } - - LogicalResult visitExpr(ConstCastOp constCast) { - // Remove any `ConstCastOp`, replacing results with inputs - constCast.getResult().replaceAllUsesWith(constCast.getInput()); - constCast->erase(); - return success(); - } - - LogicalResult visitStmt(WhenOp when) { - // Check that 'const' destinations are only connected to within - // 'const'-conditioned when blocks, unless the destination is local to the - // when block's scope - - llvm::SaveAndRestore blockSave(nonConstConditionedBlock); - bool isWithinNonconstCondition = !when.getCondition().getType().isConst(); - - if (isWithinNonconstCondition) - nonConstConditionedBlock = &when.getThenBlock(); - for (auto &op : - llvm::make_early_inc_range(llvm::reverse(when.getThenBlock()))) - if (failed(dispatchVisitor(&op))) - return failure(); - - if (when.hasElseRegion()) { - if (isWithinNonconstCondition) - nonConstConditionedBlock = &when.getElseBlock(); - for (auto &op : - llvm::make_early_inc_range(llvm::reverse(when.getElseBlock()))) - if (failed(dispatchVisitor(&op))) - return failure(); - } - - return success(); - } - - LogicalResult visitStmt(ConnectOp connect) { return handleConnect(connect); } - - LogicalResult visitStmt(StrictConnectOp connect) { - return handleConnect(connect); - } - - LogicalResult visitUnhandledOp(Operation *op) { - // Register any 'const' results to drop 'const' - for (auto [index, result] : llvm::enumerate(op->getResults())) { - if (auto convertedType = convertType(result.getType())) - result.setType(convertedType); - } - - return success(); - } - - LogicalResult visitInvalidOp(Operation *op) { return success(); } - -private: - /// Returns null type if no conversion is needed. - Type convertType(Type type) { - if (auto base = type.dyn_cast()) { - return convertType(base); - } - - if (auto refType = type.dyn_cast()) { - if (auto converted = convertType(refType.getType())) - return RefType::get(converted.cast(), - refType.getForceable()); - } - - return {}; - } - - /// Returns null type if no conversion is needed. - FIRRTLBaseType convertType(FIRRTLBaseType type) { - auto nonConstType = type.getAllConstDroppedType(); - return nonConstType != type ? nonConstType : FIRRTLBaseType{}; - } - - LogicalResult handleConnect(FConnectLike connect) { - auto dest = connect.getDest(); - auto destType = dest.getType().dyn_cast(); - if (nonConstConditionedBlock && destType && destType.containsConst()) { - // 'const' connects are allowed if `dest` is local to the non-'const' when - // block. - auto *destBlock = dest.getParentBlock(); - auto *block = connect->getBlock(); - while (block) { - // The connect is local to the `dest` declaration, both local to the - // non-'const' block, so the connect is valid. - if (block == destBlock) - return success(); - // The connect is within the non-'const' condition, non-local to the - // `dest` declaration, so the connect is invalid - if (block == nonConstConditionedBlock) - break; - - if (auto *parentOp = block->getParentOp()) - block = parentOp->getBlock(); - else - break; - } - - // A const dest is allowed if leaf elements are effectively non-const due - // to flips. - if (typeHasConstLeaf(destType)) { - if (destType.isConst()) - return connect.emitOpError() - << "assignment to 'const' type " << destType - << " is dependent on a non-'const' condition"; - return connect->emitOpError() - << "assignment to nested 'const' member of type " << destType - << " is dependent on a non-'const' condition"; - } - } - - return success(); - } - - Block *nonConstConditionedBlock = nullptr; -}; - -class DropConstPass : public DropConstBase { - void runOnOperation() override { - if (failed(OpVisitor().handleModule(getOperation()))) - signalPassFailure(); } }; } // namespace diff --git a/test/Dialect/FIRRTL/connect-errors.mlir b/test/Dialect/FIRRTL/connect-errors.mlir index 374ef8d3e6eb..aa5cc446f825 100644 --- a/test/Dialect/FIRRTL/connect-errors.mlir +++ b/test/Dialect/FIRRTL/connect-errors.mlir @@ -608,3 +608,87 @@ firrtl.module @test(in %in : !firrtl.bundle>>, !firrtl.bundle>> } } + +// ----- +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { + } else { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + +// This tests that values aren't being set to non-const before their usage is checked. +firrtl.circuit "test" { +firrtl.extmodule @Inner(in a : !firrtl.const.uint<2>) +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + %a = firrtl.instance inner @Inner(in a : !firrtl.const.uint<2>) + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %a, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } +} +} + +// ----- + +firrtl.circuit "test" { +firrtl.module @test(in %constP: !firrtl.const.uint<1>, in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.when %constP : !firrtl.const.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + } + } +} +} + +// ----- + +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> + } +} +} + +// ----- + +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.bundle>, out %out: !firrtl.const.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.bundle>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.const.bundle>, !firrtl.const.bundle> + } +} +} + +// ----- + +// Connections to flip const destinations are allowed within non-const when blocks +// when the flow is to a non-const source +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} + firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> + } +} +} diff --git a/test/Dialect/FIRRTL/connect.mlir b/test/Dialect/FIRRTL/connect.mlir index d2d01dd5a0c6..a64cb51676db 100644 --- a/test/Dialect/FIRRTL/connect.mlir +++ b/test/Dialect/FIRRTL/connect.mlir @@ -245,4 +245,84 @@ firrtl.module @ConstToNonConstDoubleFlipp(in %in : !firrtl.const.bundle>>, !firrtl.const.bundle>> } + +// Const connections can occur within const-conditioned whens +// CHECK-LABEL: firrtl.module @ConstConditionConstAssign +firrtl.module @ConstConditionConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.const.sint<2>, in %in2: !firrtl.const.sint<2>, out %out: !firrtl.const.sint<2>) { + firrtl.when %cond : !firrtl.const.uint<1> { + firrtl.strictconnect %out, %in1 : !firrtl.const.sint<2> + } else { + firrtl.strictconnect %out, %in2 : !firrtl.const.sint<2> + } +} + +// Non-const connections can occur within const-conditioned whens +// CHECK-LABEL: firrtl.module @ConstConditionNonConstAssign +firrtl.module @ConstConditionNonConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.sint<2>, in %in2: !firrtl.sint<2>, out %out: !firrtl.sint<2>) { + firrtl.when %cond : !firrtl.const.uint<1> { + firrtl.strictconnect %out, %in1 : !firrtl.sint<2> + } else { + firrtl.strictconnect %out, %in2 : !firrtl.sint<2> + } +} + +// Const connections can occur when the destination is local to a non-const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstAssign +firrtl.module @NonConstWhenLocalConstAssign(in %cond: !firrtl.uint<1>) { + firrtl.when %cond : !firrtl.uint<1> { + %w = firrtl.wire : !firrtl.const.uint<9> + %c = firrtl.constant 0 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } +} + +// Const connections can occur when the destination is local to a non-const +// conditioned when block and the connection is inside a const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign +firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign(in %cond: !firrtl.uint<1>, in %constCond: !firrtl.const.uint<1>) { + firrtl.when %cond : !firrtl.uint<1> { + %w = firrtl.wire : !firrtl.const.uint<9> + firrtl.when %constCond : !firrtl.const.uint<1> { + %c = firrtl.constant 0 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } else { + %c = firrtl.constant 1 : !firrtl.const.uint<9> + firrtl.strictconnect %w, %c : !firrtl.const.uint<9> + } + } +} + +// Connections to flip const destinations are allowed within non-const when blocks +// when the flow is to a non-const source +firrtl.module @NonConstWhenConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.const.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.connect %out, %in : !firrtl.const.bundle>, !firrtl.bundle> + } +} + +// Connections to flip const destinations are allowed within non-const when blocks +// when the flow is to a non-const source +firrtl.module @NonConstWhenNestedConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> + } +} + +// Connections to const flip sources can occur when the source is local to a non-const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstFlipAssign +firrtl.module @NonConstWhenLocalConstFlipAssign(in %cond: !firrtl.uint<1>, out %out : !firrtl.const.bundle>) { + firrtl.when %cond : !firrtl.uint<1> { + %w = firrtl.wire : !firrtl.const.bundle> + firrtl.connect %out, %w : !firrtl.const.bundle>, !firrtl.const.bundle> + } +} + +// Connections to nested const flip sources can occur when the source is local to a non-const conditioned when block +// CHECK-LABEL: firrtl.module @NonConstWhenLocalNestedConstFlipAssign +firrtl.module @NonConstWhenLocalNestedConstFlipAssign(in %cond: !firrtl.uint<1>, out %out : !firrtl.bundle>) { + firrtl.when %cond : !firrtl.uint<1> { + %w = firrtl.wire : !firrtl.bundle> + firrtl.connect %out, %w : !firrtl.bundle>, !firrtl.bundle> + } +} } diff --git a/test/Dialect/FIRRTL/drop-const-errors.mlir b/test/Dialect/FIRRTL/drop-const-errors.mlir deleted file mode 100644 index 549f347e3070..000000000000 --- a/test/Dialect/FIRRTL/drop-const-errors.mlir +++ /dev/null @@ -1,60 +0,0 @@ -// RUN: circt-opt --pass-pipeline='builtin.module(firrtl.circuit(any(firrtl-drop-const)))' -verify-diagnostics --split-input-file %s - -firrtl.circuit "CheckConstAssignInNonConstCondition" { -firrtl.module @CheckConstAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - firrtl.when %p : !firrtl.uint<1> { - // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> - } -} -} - -// ----- - -firrtl.circuit "CheckConstAssignInNonConstConditionElse" { -firrtl.module @CheckConstAssignInNonConstConditionElse(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - firrtl.when %p : !firrtl.uint<1> { - } else { - // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> - } -} -} - -// ----- - -// This tests that values aren't being set to non-const before their usage is checked. -firrtl.circuit "CheckIntermediateValueConstAssignInNonConstCondition" { -firrtl.extmodule @Inner(in a : !firrtl.const.uint<2>) -firrtl.module @CheckIntermediateValueConstAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - %a = firrtl.instance inner @Inner(in a : !firrtl.const.uint<2>) - firrtl.when %p : !firrtl.uint<1> { - // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %a, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> - } -} -} - -// ----- - -firrtl.circuit "CheckNestedConstAssignInNonConstCondition" { -firrtl.module @CheckNestedConstAssignInNonConstCondition(in %constP: !firrtl.const.uint<1>, in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - firrtl.when %p : !firrtl.uint<1> { - firrtl.when %constP : !firrtl.const.uint<1> { - // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> - } - } -} -} - -// ----- - -firrtl.circuit "CheckBundleConstFieldAssignInNonConstCondition" { -firrtl.module @CheckBundleConstFieldAssignInNonConstCondition(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { - firrtl.when %p : !firrtl.uint<1> { - // expected-error @+1 {{assignment to nested 'const' member of type '!firrtl.bundle>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> - } -} -} diff --git a/test/Dialect/FIRRTL/drop-const.mlir b/test/Dialect/FIRRTL/drop-const.mlir index dd5bad45793a..3aab8838ec8c 100644 --- a/test/Dialect/FIRRTL/drop-const.mlir +++ b/test/Dialect/FIRRTL/drop-const.mlir @@ -82,9 +82,9 @@ firrtl.module @ConstCastErase(in %in: !firrtl.const.uint<1>, out %out: !firrtl.u firrtl.strictconnect %out, %0 : !firrtl.uint<1> } -// Const connections can occur within const-conditioned whens -// CHECK-LABEL: firrtl.module @ConstConditionConstAssign -firrtl.module @ConstConditionConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.const.sint<2>, in %in2: !firrtl.const.sint<2>, out %out: !firrtl.const.sint<2>) { +// Const is dropped within when blocks +// CHECK-LABEL: firrtl.module @ConstDropInWhenBlock +firrtl.module @ConstDropInWhenBlock(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.const.sint<2>, in %in2: !firrtl.const.sint<2>, out %out: !firrtl.const.sint<2>) { // CHECK: firrtl.when %cond : !firrtl.uint<1> firrtl.when %cond : !firrtl.const.uint<1> { // CHECK: firrtl.strictconnect %out, %in1 : !firrtl.sint<2> @@ -94,57 +94,4 @@ firrtl.module @ConstConditionConstAssign(in %cond: !firrtl.const.uint<1>, in %in firrtl.strictconnect %out, %in2 : !firrtl.const.sint<2> } } - -// Non-const connections can occur within const-conditioned whens -// CHECK-LABEL: firrtl.module @ConstConditionNonConstAssign -firrtl.module @ConstConditionNonConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.sint<2>, in %in2: !firrtl.sint<2>, out %out: !firrtl.sint<2>) { - // CHECK: firrtl.when %cond : !firrtl.uint<1> - firrtl.when %cond : !firrtl.const.uint<1> { - firrtl.strictconnect %out, %in1 : !firrtl.sint<2> - } else { - firrtl.strictconnect %out, %in2 : !firrtl.sint<2> - } -} - -// Const connections can occur when the destination is local to a non-const conditioned when block -// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstAssign -firrtl.module @NonConstWhenLocalConstAssign(in %cond: !firrtl.uint<1>) { - firrtl.when %cond : !firrtl.uint<1> { - // CHECK: firrtl.wire : !firrtl.uint<9> - // CHECK-NEXT: firrtl.constant 0 : !firrtl.uint<9> - %w = firrtl.wire : !firrtl.const.uint<9> - %c = firrtl.constant 0 : !firrtl.const.uint<9> - firrtl.strictconnect %w, %c : !firrtl.const.uint<9> - } -} - -// Const connections can occur when the destination is local to a non-const -// conditioned when block and the connection is inside a const conditioned when block -// CHECK-LABEL: firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign -firrtl.module @NonConstWhenLocalConstNestedConstWhenAssign(in %cond: !firrtl.uint<1>, in %constCond: !firrtl.const.uint<1>) { - firrtl.when %cond : !firrtl.uint<1> { - // CHECK: firrtl.wire : !firrtl.uint<9> - %w = firrtl.wire : !firrtl.const.uint<9> - // CHECK-NEXT: firrtl.when %constCond : !firrtl.uint<1> - firrtl.when %constCond : !firrtl.const.uint<1> { - %c = firrtl.constant 0 : !firrtl.const.uint<9> - firrtl.strictconnect %w, %c : !firrtl.const.uint<9> - } else { - %c = firrtl.constant 1 : !firrtl.const.uint<9> - firrtl.strictconnect %w, %c : !firrtl.const.uint<9> - } - } -} - -firrtl.module @NonConstWhenConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.const.bundle>) { - firrtl.when %p : !firrtl.uint<1> { - firrtl.connect %out, %in : !firrtl.const.bundle>, !firrtl.bundle> - } -} - -firrtl.module @NonConstWhenNestedConstFlipAssign(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { - firrtl.when %p : !firrtl.uint<1> { - firrtl.connect %out, %in : !firrtl.bundle>, !firrtl.bundle> - } -} } \ No newline at end of file From 92cdb80467727f6cdc298e8acb96c55c1f9545bf Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Fri, 19 May 2023 10:02:04 -0600 Subject: [PATCH 06/11] Move getAllConstDroppedType into DropConst pass --- include/circt/Dialect/FIRRTL/FIRRTLTypes.h | 3 -- .../circt/Dialect/FIRRTL/FIRRTLTypesImpl.td | 9 ---- lib/Dialect/FIRRTL/FIRRTLTypes.cpp | 44 ------------------ lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 45 +++++++++++++++++-- 4 files changed, 41 insertions(+), 60 deletions(-) diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h index 6f626835b8d1..ee73cb71a1a5 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h @@ -135,9 +135,6 @@ class FIRRTLBaseType /// Return a 'const' or non-'const' version of this type. FIRRTLBaseType getConstType(bool isConst); - /// Return this type with a 'const' modifiers dropped - FIRRTLBaseType getAllConstDroppedType(); - /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType getMaskType(); diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td index 1be237c48b7f..d628573cd3ba 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td @@ -212,9 +212,6 @@ def FVectorImpl : BaseVectorTypeImpl<"FVector","::circt::firrtl::FIRRTLBaseType" let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); - - /// Return this type with a 'const' modifiers dropped - FVectorType getAllConstDroppedType(); }]; } @@ -334,9 +331,6 @@ def BundleImpl : BaseBundleTypeImpl<"Bundle","::circt::firrtl::FIRRTLBaseType", let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); - - /// Return this type with a 'const' modifiers dropped - BundleType getAllConstDroppedType(); }]; } @@ -378,9 +372,6 @@ def FEnumImpl : FIRRTLImplType<"FEnum", [FieldIDTypeInterface]> { FEnumType getConstType(bool isConst); - /// Return this type with a 'const' modifiers dropped - FEnumType getAllConstDroppedType(); - /// Look up an element's index by name. This returns None on failure. std::optional getElementIndex(StringAttr name); std::optional getElementIndex(StringRef name); diff --git a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp index 3bb6e093beef..9490827d915e 100644 --- a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp @@ -582,19 +582,6 @@ FIRRTLBaseType FIRRTLBaseType::getConstType(bool isConst) { }); } -/// Return this type with a 'const' modifiers dropped -FIRRTLBaseType FIRRTLBaseType::getAllConstDroppedType() { - return TypeSwitch(*this) - .Case([&](auto type) { return type.getConstType(false); }) - .Case( - [&](auto type) { return type.getAllConstDroppedType(); }) - .Default([](Type) { - llvm_unreachable("unknown FIRRTL type"); - return FIRRTLBaseType(); - }); -} - /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType FIRRTLBaseType::getMaskType() { @@ -1233,18 +1220,6 @@ BundleType BundleType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } -BundleType BundleType::getAllConstDroppedType() { - if (!containsConst()) - return *this; - - SmallVector constDroppedElements( - llvm::map_range(getElements(), [](BundleElement element) { - element.type = element.type.getAllConstDroppedType(); - return element; - })); - return get(getContext(), constDroppedElements, false); -} - std::optional BundleType::getElementIndex(StringAttr name) { for (const auto &it : llvm::enumerate(getElements())) { auto element = it.value(); @@ -1639,13 +1614,6 @@ FVectorType FVectorType::getConstType(bool isConst) { return get(getElementType(), getNumElements(), isConst); } -FVectorType FVectorType::getAllConstDroppedType() { - if (!containsConst()) - return *this; - return get(getElementType().getAllConstDroppedType(), getNumElements(), - false); -} - uint64_t FVectorType::getFieldID(uint64_t index) { return 1 + index * (getElementType().getMaxFieldID() + 1); } @@ -1877,18 +1845,6 @@ FEnumType FEnumType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } -FEnumType FEnumType::getAllConstDroppedType() { - if (!containsConst()) - return *this; - - SmallVector constDroppedElements( - llvm::map_range(getElements(), [](EnumElement element) { - element.type = element.type.getAllConstDroppedType(); - return element; - })); - return get(getContext(), constDroppedElements, false); -} - /// Return a pair with the 'isPassive' and 'containsAnalog' bits. RecursiveTypeProperties FEnumType::getRecursiveTypeProperties() const { return getImpl()->recProps; diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index efea651f4d94..785f2a9e8426 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -23,9 +23,47 @@ using namespace circt; using namespace firrtl; +// NOLINTBEGIN(misc-no-recursion) +/// Return this type with a 'const' modifiers dropped +static FIRRTLBaseType getAllConstDroppedType(FIRRTLBaseType type) { + if (!type.containsConst()) + return type; + return TypeSwitch(type) + .Case([](auto type) { return type.getConstType(false); }) + .Case([](BundleType type) { + SmallVector constDroppedElements( + llvm::map_range( + type.getElements(), [](BundleType::BundleElement element) { + element.type = getAllConstDroppedType(element.type); + return element; + })); + return BundleType::get(type.getContext(), constDroppedElements, false); + }) + .Case([](FVectorType type) { + return FVectorType::get(getAllConstDroppedType(type.getElementType()), + type.getNumElements(), false); + }) + .Case([](FEnumType type) { + SmallVector constDroppedElements( + llvm::map_range( + type.getElements(), [](FEnumType::EnumElement element) { + element.type = getAllConstDroppedType(element.type); + return element; + })); + return FEnumType::get(type.getContext(), constDroppedElements, false); + ; + }) + .Default([](Type) { + llvm_unreachable("unknown FIRRTL type"); + return FIRRTLBaseType(); + }); +} +// NOLINTEND(misc-no-recursion) + /// Returns null type if no conversion is needed. static FIRRTLBaseType convertType(FIRRTLBaseType type) { - auto nonConstType = type.getAllConstDroppedType(); + auto nonConstType = getAllConstDroppedType(type); return nonConstType != type ? nonConstType : FIRRTLBaseType{}; } @@ -37,8 +75,7 @@ static Type convertType(Type type) { if (auto refType = type.dyn_cast()) { if (auto converted = convertType(refType.getType())) - return RefType::get(converted.cast(), - refType.getForceable()); + return RefType::get(converted, refType.getForceable()); } return {}; @@ -50,7 +87,7 @@ class DropConstPass : public DropConstBase { auto module = getOperation(); auto fmodule = dyn_cast(*module); - // Check the module body if present + // Convert the module body if present if (fmodule) { fmodule->walk([](Operation *op) { if (auto constCastOp = dyn_cast(op)) { From a339df3481278d0f3f014580d48c6636ad192e47 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Tue, 23 May 2023 23:47:40 -0600 Subject: [PATCH 07/11] Pr updates --- .../circt/Dialect/FIRRTL/FIRRTLTypesImpl.td | 10 +- include/circt/Dialect/FIRRTL/Passes.td | 4 +- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 97 ++++++++++++------- lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 65 ++++++------- test/Dialect/FIRRTL/connect-errors.mlir | 63 +++++++++--- test/Dialect/FIRRTL/connect.mlir | 12 ++- 6 files changed, 160 insertions(+), 91 deletions(-) diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td index d628573cd3ba..201a93981bac 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td @@ -22,12 +22,12 @@ class FIRRTLImplType traits = [], string baseCppClass = "::circt::firrtl::FIRRTLBaseType"> : TypeDef { - // Storage classes must be defined in C++ and + // Storage classes must be defined in C++ and // inherit from FIRRTLBaseTypeStorage let genStorageClass = false; - - // MLIR generates awkward accessor "getIsConst" for the "isConst" parameter, - // which is common on FIRRTLBaseType anyway, so we generate the other + + // MLIR generates awkward accessor "getIsConst" for the "isConst" parameter, + // which is common on FIRRTLBaseType anyway, so we generate the other // accessors manually let genAccessors = false; } @@ -450,7 +450,7 @@ def RefImpl : FIRRTLImplType<"Ref", Values of this type are used to capture dataflow paths, and do not represent a circuit element or entity. - + Generally read-only (probe), optionally forceable (rwprobe). }]; let parameters = (ins TypeParameter<"::circt::firrtl::FIRRTLBaseType", diff --git a/include/circt/Dialect/FIRRTL/Passes.td b/include/circt/Dialect/FIRRTL/Passes.td index 5eba71eec21a..e7949df84c6e 100644 --- a/include/circt/Dialect/FIRRTL/Passes.td +++ b/include/circt/Dialect/FIRRTL/Passes.td @@ -97,8 +97,8 @@ def IMConstProp : Pass<"firrtl-imconstprop", "firrtl::CircuitOp"> { def RegisterOptimizer : Pass<"firrtl-register-optimizer", "firrtl::FModuleOp"> { let summary = "Optimizer Registers"; let description = [{ - This pass applies classic FIRRTL register optimizations. These - optimizations are isolated to this pass as they can change the visible + This pass applies classic FIRRTL register optimizations. These + optimizations are isolated to this pass as they can change the visible behavior of the register, especially before reset. }]; let constructor = "circt::firrtl::createRegisterOptimizerPass()"; diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index fdd6d3fc36cc..0e120d5b836b 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -2331,37 +2331,52 @@ static LogicalResult checkConnectFlow(Operation *connect) { return success(); } -namespace { -enum class ConstLeafKind : uint8_t { None = 0, Normal = 1 << 0, Flip = 1 << 1 }; -} // end anonymous namespace - // NOLINTBEGIN(misc-no-recursion) -/// Checks if the type has any 'const' leaf elements that are normal or flip -static ConstLeafKind typeConstLeafKinds(FIRRTLBaseType type, - bool outerTypeIsConst = false, - bool isFlip = false) { +/// Checks if the type has any 'const' leaf elements . If `isFlip` is `true`, +/// the `const` leaf is not considered to be driven. +static bool isConstFieldDriven(FIRRTLBaseType type, bool isFlip = false, + bool outerTypeIsConst = false) { auto typeIsConst = outerTypeIsConst || type.isConst(); if (typeIsConst && type.isPassive()) - return isFlip ? ConstLeafKind::Flip : ConstLeafKind::Normal; - - if (auto bundleType = type.dyn_cast()) { - auto kinds = ConstLeafKind::None; - for (auto &element : bundleType.getElements()) { - auto elementKinds = typeConstLeafKinds(element.type, typeIsConst, - isFlip ^ element.isFlip); - kinds = static_cast(static_cast(kinds) | - static_cast(elementKinds)); - } - return kinds; - } + return !isFlip; + + if (auto bundleType = dyn_cast(type)) + return llvm::any_of(bundleType.getElements(), [&](auto &element) { + return isConstFieldDriven(element.type, isFlip ^ element.isFlip, + typeIsConst); + }); if (auto vectorType = type.dyn_cast()) - return typeConstLeafKinds(vectorType.getElementType(), typeIsConst, isFlip); + return isConstFieldDriven(vectorType.getElementType(), isFlip, typeIsConst); if (typeIsConst) - return isFlip ? ConstLeafKind::Normal : ConstLeafKind::Flip; - return ConstLeafKind::None; + return !isFlip; + return false; +} + +/// Looks up the value's defining op until the defining op is null or a +/// declaration of the value. If a SubAccessOp is encountered with a 'const' +/// input, `originalFieldType` is made 'const'. +static Value +findFieldDeclarationRefiningFieldType(Value value, + FIRRTLBaseType &originalFieldType) { + auto *definingOp = value.getDefiningOp(); + if (!definingOp) + return value; + + return TypeSwitch(definingOp) + .Case([&](auto op) { + return findFieldDeclarationRefiningFieldType(op.getInput(), + originalFieldType); + }) + .Case([&](SubaccessOp op) { + if (op.getInput().getType().getElementTypePreservingConst().isConst()) + originalFieldType = originalFieldType.getConstType(true); + return findFieldDeclarationRefiningFieldType(op.getInput(), + originalFieldType); + }) + .Default([&](Operation *) { return value; }); } // NOLINTEND(misc-no-recursion) @@ -2375,11 +2390,26 @@ static LogicalResult checkConnectConditionality(FConnectLike connect) { if (!destType || !srcType) return success(); - auto checkConstConditionality = [&](Value value, - FIRRTLBaseType type) -> LogicalResult { - auto *definingBlock = value.getParentBlock(); + auto destRefinedType = destType; + auto srcRefinedType = srcType; + + auto destDeclaration = + findFieldDeclarationRefiningFieldType(dest, destRefinedType); + auto srcDeclaration = + findFieldDeclarationRefiningFieldType(src, srcRefinedType); + + // Make sure subaccesses don't allow for a non-'const' to 'const' connection + if ((destType != destRefinedType || srcType != srcRefinedType) && + !areTypesEquivalent(destRefinedType, srcRefinedType)) { + return connect.emitError("type mismatch between destination ") + << destRefinedType << " and source " << srcRefinedType; + } + + auto checkConstConditionality = [&](Value value, FIRRTLBaseType type, + Value declaration) -> LogicalResult { + auto *declarationBlock = declaration.getParentBlock(); auto *block = connect->getBlock(); - while (block && block != definingBlock) { + while (block && block != declarationBlock) { auto *parentOp = block->getParentOp(); if (auto whenOp = dyn_cast(parentOp); @@ -2399,17 +2429,14 @@ static LogicalResult checkConnectConditionality(FConnectLike connect) { }; // Check destination if it contains 'const' leaves - if (destType.containsConst() && - static_cast(typeConstLeafKinds(destType)) & - static_cast(ConstLeafKind::Normal) && - failed(checkConstConditionality(dest, destType))) + if (destRefinedType.containsConst() && isConstFieldDriven(destRefinedType) && + failed(checkConstConditionality(dest, destType, destDeclaration))) return failure(); // Check source if it contains 'const' 'flip' leaves - if (srcType.containsConst() && - static_cast(typeConstLeafKinds(srcType)) & - static_cast(ConstLeafKind::Flip) && - failed(checkConstConditionality(src, srcType))) + if (srcRefinedType.containsConst() && + isConstFieldDriven(srcRefinedType, /*isFlip=*/true) && + failed(checkConstConditionality(src, srcType, srcDeclaration))) return failure(); return success(); diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index 785f2a9e8426..33f01b64e017 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -14,11 +14,7 @@ #include "circt/Dialect/FIRRTL/FIRRTLOps.h" #include "circt/Dialect/FIRRTL/FIRRTLTypes.h" #include "circt/Dialect/FIRRTL/FIRRTLUtils.h" -#include "circt/Dialect/FIRRTL/FIRRTLVisitors.h" #include "circt/Dialect/FIRRTL/Passes.h" -#include "llvm/Support/SaveAndRestore.h" - -#define DEBUG_TYPE "drop-const" using namespace circt; using namespace firrtl; @@ -52,7 +48,6 @@ static FIRRTLBaseType getAllConstDroppedType(FIRRTLBaseType type) { return element; })); return FEnumType::get(type.getContext(), constDroppedElements, false); - ; }) .Default([](Type) { llvm_unreachable("unknown FIRRTL type"); @@ -85,40 +80,44 @@ namespace { class DropConstPass : public DropConstBase { void runOnOperation() override { auto module = getOperation(); - auto fmodule = dyn_cast(*module); // Convert the module body if present - if (fmodule) { - fmodule->walk([](Operation *op) { - if (auto constCastOp = dyn_cast(op)) { - // Remove any `ConstCastOp`, replacing results with inputs - constCastOp.getResult().replaceAllUsesWith(constCastOp.getInput()); - constCastOp->erase(); - return; - } + module->walk([](Operation *op) { + if (auto constCastOp = dyn_cast(op)) { + // Remove any `ConstCastOp`, replacing results with inputs + constCastOp.getResult().replaceAllUsesWith(constCastOp.getInput()); + constCastOp->erase(); + return; + } - for (auto result : op->getResults()) { - if (auto convertedType = convertType(result.getType())) - result.setType(convertedType); - } - }); - } + // Convert any block arguments + for (auto ®ion : op->getRegions()) + for (auto &block : region.getBlocks()) + for (auto argument : block.getArguments()) + if (auto convertedType = convertType(argument.getType())) + argument.setType(convertedType); - // Find 'const' ports - auto portTypes = SmallVector(module.getPortTypes()); - for (size_t portIndex = 0, numPorts = module.getPortTypes().size(); - portIndex < numPorts; ++portIndex) { - if (auto convertedType = convertType(module.getPortType(portIndex))) { - // If this is an FModuleOp, register the block argument to drop 'const' - if (fmodule) - fmodule.getArgument(portIndex).setType(convertedType); - portTypes[portIndex] = TypeAttr::get(convertedType); - } - } + for (auto result : op->getResults()) + if (auto convertedType = convertType(result.getType())) + result.setType(convertedType); + }); // Update the module signature with non-'const' ports - module->setAttr(FModuleLike::getPortTypesAttrName(), - ArrayAttr::get(module.getContext(), portTypes)); + SmallVector portTypes; + portTypes.reserve(module.getNumPorts()); + bool convertedAny = false; + llvm::transform(module.getPortTypes(), std::back_inserter(portTypes), + [&](Attribute type) -> Attribute { + if (auto convertedType = + convertType(type.cast().getValue())) { + convertedAny = true; + return TypeAttr::get(convertedType); + } + return type; + }); + if (convertedAny) + module->setAttr(FModuleLike::getPortTypesAttrName(), + ArrayAttr::get(module.getContext(), portTypes)); } }; } // namespace diff --git a/test/Dialect/FIRRTL/connect-errors.mlir b/test/Dialect/FIRRTL/connect-errors.mlir index aa5cc446f825..086fd92bf925 100644 --- a/test/Dialect/FIRRTL/connect-errors.mlir +++ b/test/Dialect/FIRRTL/connect-errors.mlir @@ -551,7 +551,7 @@ firrtl.module @test(in %a : !firrtl.uint<1>, out %b : !firrtl.sint<1>) { // ----- -/// Non-const types cannot be connected to const types. +// Non-const types cannot be connected to const types. firrtl.circuit "test" { firrtl.module @test(in %a : !firrtl.uint<1>, out %b : !firrtl.const.uint<1>) { @@ -586,7 +586,7 @@ firrtl.module @test(in %in : !firrtl.const.bundle>, // ----- -/// Nested const flip types cannot be connected to non-const flip types. +// Nested const flip types cannot be connected to non-const flip types. firrtl.circuit "test" { firrtl.module @test(in %in : !firrtl.bundle>, @@ -610,21 +610,32 @@ firrtl.module @test(in %in : !firrtl.bundle>>, } // ----- + +// Test that subaccess of a const vector is checked as if the field is const. firrtl.circuit "test" { -firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - firrtl.when %p : !firrtl.uint<1> { - // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> - } +firrtl.module @test(in %index: !firrtl.uint<1>, out %out: !firrtl.const.vector, 1>) { + %c = firrtl.constant 0 : !firrtl.uint<1> + %d = firrtl.subaccess %out[%index] : !firrtl.const.vector, 1>, !firrtl.uint<1> + // expected-error @+1 {{type mismatch}} + firrtl.strictconnect %d, %c : !firrtl.uint<1> } } // ----- +// Test that subaccess of a flipped const vector is checked as if the field is const. +firrtl.circuit "test" { +firrtl.module @test(in %index: !firrtl.uint<1>, in %in: !firrtl.const.vector>, 1>, out %out: !firrtl.bundle>) { + %element = firrtl.subaccess %in[%index] : !firrtl.const.vector>, 1>, !firrtl.uint<1> + // expected-error @+1 {{type mismatch}} + firrtl.connect %out, %element : !firrtl.bundle>, !firrtl.bundle> +} +} + +// ----- firrtl.circuit "test" { firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { firrtl.when %p : !firrtl.uint<1> { - } else { // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> } @@ -633,14 +644,12 @@ firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out % // ----- -// This tests that values aren't being set to non-const before their usage is checked. firrtl.circuit "test" { -firrtl.extmodule @Inner(in a : !firrtl.const.uint<2>) firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) { - %a = firrtl.instance inner @Inner(in a : !firrtl.const.uint<2>) firrtl.when %p : !firrtl.uint<1> { + } else { // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<2>' is dependent on a non-'const' condition}} - firrtl.connect %a, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> + firrtl.connect %out, %in : !firrtl.const.uint<2>, !firrtl.const.uint<2> } } } @@ -682,8 +691,6 @@ firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.bundle, in %in: !firrtl.bundle>, out %out: !firrtl.bundle>) { firrtl.when %p : !firrtl.uint<1> { @@ -692,3 +699,31 @@ firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.bundle, out %out: !firrtl.const.bundle>) { + firrtl.when %p : !firrtl.uint<1> { + %f = firrtl.subfield %out[a] : !firrtl.const.bundle> + %c = firrtl.constant 0 : !firrtl.const.uint<1> + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<1>' is dependent on a non-'const' condition}} + firrtl.strictconnect %f, %c : !firrtl.const.uint<1> + } +} +} + +// ----- + +// Test that the declaration location of the vector containing the field is checked. +firrtl.circuit "test" { +firrtl.module @test(in %p: !firrtl.uint<1>, out %out: !firrtl.const.vector, 1>) { + firrtl.when %p : !firrtl.uint<1> { + %e = firrtl.subindex %out[0] : !firrtl.const.vector, 1> + %c = firrtl.constant 0 : !firrtl.const.uint<1> + // expected-error @+1 {{assignment to 'const' type '!firrtl.const.uint<1>' is dependent on a non-'const' condition}} + firrtl.strictconnect %e, %c : !firrtl.const.uint<1> + } +} +} diff --git a/test/Dialect/FIRRTL/connect.mlir b/test/Dialect/FIRRTL/connect.mlir index a64cb51676db..62cc0f2b71ac 100644 --- a/test/Dialect/FIRRTL/connect.mlir +++ b/test/Dialect/FIRRTL/connect.mlir @@ -238,14 +238,22 @@ firrtl.module @NonConstToNestedConstFlip(in %in : !firrtl.bundle>, !firrtl.bundle> } -firrtl.module @ConstToNonConstDoubleFlipp(in %in : !firrtl.const.bundle>>, - out %out : !firrtl.bundle>>) { +firrtl.module @ConstToNonConstDoubleFlip(in %in : !firrtl.const.bundle>>, + out %out : !firrtl.bundle>>) { // CHECK: firrtl.connect %out, %in : // CHECK-SAME: !firrtl.bundle>>, !firrtl.const.bundle>> firrtl.connect %out, %in : !firrtl.bundle>>, !firrtl.const.bundle>> } +firrtl.module @NonConstToNonConstFlipFromConstSubaccess(in %in : !firrtl.bundle>, + out %out : !firrtl.const.vector>, 1>, + in %index : !firrtl.uint<1>) { + %0 = firrtl.subaccess %out[%index] : !firrtl.const.vector>, 1>, !firrtl.uint<1> + // CHECK: firrtl.connect %0, %in : !firrtl.bundle>, !firrtl.bundle> + firrtl.connect %0, %in : !firrtl.bundle>, !firrtl.bundle> +} + // Const connections can occur within const-conditioned whens // CHECK-LABEL: firrtl.module @ConstConditionConstAssign firrtl.module @ConstConditionConstAssign(in %cond: !firrtl.const.uint<1>, in %in1: !firrtl.const.sint<2>, in %in2: !firrtl.const.sint<2>, out %out: !firrtl.const.sint<2>) { From 72561b853d5bc49993cb6d6c912597d6f6008a59 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Wed, 24 May 2023 10:03:48 -0600 Subject: [PATCH 08/11] Revert moving getAllConstDroppedType --- include/circt/Dialect/FIRRTL/FIRRTLTypes.h | 3 ++ .../circt/Dialect/FIRRTL/FIRRTLTypesImpl.td | 9 ++++ lib/Dialect/FIRRTL/FIRRTLTypes.cpp | 44 +++++++++++++++++++ lib/Dialect/FIRRTL/Transforms/DropConst.cpp | 39 +--------------- 4 files changed, 57 insertions(+), 38 deletions(-) diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h index ee73cb71a1a5..6f626835b8d1 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypes.h +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypes.h @@ -135,6 +135,9 @@ class FIRRTLBaseType /// Return a 'const' or non-'const' version of this type. FIRRTLBaseType getConstType(bool isConst); + /// Return this type with a 'const' modifiers dropped + FIRRTLBaseType getAllConstDroppedType(); + /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType getMaskType(); diff --git a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td index 201a93981bac..537e0b33f334 100644 --- a/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td +++ b/include/circt/Dialect/FIRRTL/FIRRTLTypesImpl.td @@ -212,6 +212,9 @@ def FVectorImpl : BaseVectorTypeImpl<"FVector","::circt::firrtl::FIRRTLBaseType" let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); + + /// Return this type with a 'const' modifiers dropped + FVectorType getAllConstDroppedType(); }]; } @@ -331,6 +334,9 @@ def BundleImpl : BaseBundleTypeImpl<"Bundle","::circt::firrtl::FIRRTLBaseType", let firrtlExtraClassDeclaration = [{ /// Return this type with any flip types recursively removed from itself. FIRRTLBaseType getPassiveType(); + + /// Return this type with a 'const' modifiers dropped + BundleType getAllConstDroppedType(); }]; } @@ -372,6 +378,9 @@ def FEnumImpl : FIRRTLImplType<"FEnum", [FieldIDTypeInterface]> { FEnumType getConstType(bool isConst); + /// Return this type with a 'const' modifiers dropped + FEnumType getAllConstDroppedType(); + /// Look up an element's index by name. This returns None on failure. std::optional getElementIndex(StringAttr name); std::optional getElementIndex(StringRef name); diff --git a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp index 9490827d915e..3bb6e093beef 100644 --- a/lib/Dialect/FIRRTL/FIRRTLTypes.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLTypes.cpp @@ -582,6 +582,19 @@ FIRRTLBaseType FIRRTLBaseType::getConstType(bool isConst) { }); } +/// Return this type with a 'const' modifiers dropped +FIRRTLBaseType FIRRTLBaseType::getAllConstDroppedType() { + return TypeSwitch(*this) + .Case([&](auto type) { return type.getConstType(false); }) + .Case( + [&](auto type) { return type.getAllConstDroppedType(); }) + .Default([](Type) { + llvm_unreachable("unknown FIRRTL type"); + return FIRRTLBaseType(); + }); +} + /// Return this type with all ground types replaced with UInt<1>. This is /// used for `mem` operations. FIRRTLBaseType FIRRTLBaseType::getMaskType() { @@ -1220,6 +1233,18 @@ BundleType BundleType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } +BundleType BundleType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + + SmallVector constDroppedElements( + llvm::map_range(getElements(), [](BundleElement element) { + element.type = element.type.getAllConstDroppedType(); + return element; + })); + return get(getContext(), constDroppedElements, false); +} + std::optional BundleType::getElementIndex(StringAttr name) { for (const auto &it : llvm::enumerate(getElements())) { auto element = it.value(); @@ -1614,6 +1639,13 @@ FVectorType FVectorType::getConstType(bool isConst) { return get(getElementType(), getNumElements(), isConst); } +FVectorType FVectorType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + return get(getElementType().getAllConstDroppedType(), getNumElements(), + false); +} + uint64_t FVectorType::getFieldID(uint64_t index) { return 1 + index * (getElementType().getMaxFieldID() + 1); } @@ -1845,6 +1877,18 @@ FEnumType FEnumType::getConstType(bool isConst) { return get(getContext(), getElements(), isConst); } +FEnumType FEnumType::getAllConstDroppedType() { + if (!containsConst()) + return *this; + + SmallVector constDroppedElements( + llvm::map_range(getElements(), [](EnumElement element) { + element.type = element.type.getAllConstDroppedType(); + return element; + })); + return get(getContext(), constDroppedElements, false); +} + /// Return a pair with the 'isPassive' and 'containsAnalog' bits. RecursiveTypeProperties FEnumType::getRecursiveTypeProperties() const { return getImpl()->recProps; diff --git a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp index 33f01b64e017..7b73e4c4cf1d 100644 --- a/lib/Dialect/FIRRTL/Transforms/DropConst.cpp +++ b/lib/Dialect/FIRRTL/Transforms/DropConst.cpp @@ -19,46 +19,9 @@ using namespace circt; using namespace firrtl; -// NOLINTBEGIN(misc-no-recursion) -/// Return this type with a 'const' modifiers dropped -static FIRRTLBaseType getAllConstDroppedType(FIRRTLBaseType type) { - if (!type.containsConst()) - return type; - return TypeSwitch(type) - .Case([](auto type) { return type.getConstType(false); }) - .Case([](BundleType type) { - SmallVector constDroppedElements( - llvm::map_range( - type.getElements(), [](BundleType::BundleElement element) { - element.type = getAllConstDroppedType(element.type); - return element; - })); - return BundleType::get(type.getContext(), constDroppedElements, false); - }) - .Case([](FVectorType type) { - return FVectorType::get(getAllConstDroppedType(type.getElementType()), - type.getNumElements(), false); - }) - .Case([](FEnumType type) { - SmallVector constDroppedElements( - llvm::map_range( - type.getElements(), [](FEnumType::EnumElement element) { - element.type = getAllConstDroppedType(element.type); - return element; - })); - return FEnumType::get(type.getContext(), constDroppedElements, false); - }) - .Default([](Type) { - llvm_unreachable("unknown FIRRTL type"); - return FIRRTLBaseType(); - }); -} -// NOLINTEND(misc-no-recursion) - /// Returns null type if no conversion is needed. static FIRRTLBaseType convertType(FIRRTLBaseType type) { - auto nonConstType = getAllConstDroppedType(type); + auto nonConstType = type.getAllConstDroppedType(); return nonConstType != type ? nonConstType : FIRRTLBaseType{}; } From 56f4c505c3ec9a5d9ebe1afd3ff52378db0c09f0 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Wed, 24 May 2023 10:10:35 -0600 Subject: [PATCH 09/11] Have SubAccessOp drop all const on dynamic index --- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 6 +++--- test/Dialect/FIRRTL/const.mlir | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index 0e120d5b836b..b74020e94be5 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -3430,9 +3430,9 @@ FIRRTLType SubaccessOp::inferReturnType(ValueRange operands, indexType); if (auto vectorType = inType.dyn_cast()) { - auto elementType = vectorType.getElementType(); - return elementType.getConstType( - (elementType.isConst() || vectorType.isConst()) && isConst(indexType)); + if (isConst(indexType)) + return vectorType.getElementTypePreservingConst(); + return vectorType.getElementType().getAllConstDroppedType(); } return emitInferRetTypeError(loc, "subaccess requires vector operand, not ", diff --git a/test/Dialect/FIRRTL/const.mlir b/test/Dialect/FIRRTL/const.mlir index b176e3797f2d..5b720fb7768e 100644 --- a/test/Dialect/FIRRTL/const.mlir +++ b/test/Dialect/FIRRTL/const.mlir @@ -105,6 +105,21 @@ firrtl.module @ConstElementSubaccess(in %a: !firrtl.vector, 3>, in firrtl.connect %dynamicOut, %1 : !firrtl.uint<1>, !firrtl.uint<1> } +// Subaccess of a non-const vector with a nested const element type should preserve the subelement constness +// only if the index is const +// CHECK-LABEL: firrtl.module @ConstNestedElementSubaccess +firrtl.module @ConstNestedElementSubaccess(in %a: !firrtl.vector>, 3>, in %constIndex: !firrtl.const.uint<4>, in %dynamicIndex: !firrtl.uint<4>) { + // CHECK-NEXT: [[VAL0:%.+]] = firrtl.subaccess %a[%constIndex] : !firrtl.vector>, 3>, !firrtl.const.uint<4> + // CHECK-NEXT: [[VAL1:%.+]] = firrtl.subfield [[VAL0]][a] : !firrtl.bundle> + %0 = firrtl.subaccess %a[%constIndex] : !firrtl.vector>, 3>, !firrtl.const.uint<4> + %1 = firrtl.subfield %0[a] : !firrtl.bundle> + + // CHECK-NEXT: [[VAL2:%.+]] = firrtl.subaccess %a[%dynamicIndex] : !firrtl.vector>, 3>, !firrtl.uint<4> + // CHECK-NEXT: [[VAL3:%.+]] = firrtl.subfield [[VAL2]][a] : !firrtl.bundle> + %2 = firrtl.subaccess %a[%dynamicIndex] : !firrtl.vector>, 3>, !firrtl.uint<4> + %3 = firrtl.subfield %2[a] : !firrtl.bundle> +} + // CHECK-LABEL: firrtl.module @ConstSubtag firrtl.module @ConstSubtag(in %in : !firrtl.const.enum, b: uint<2>>, out %out : !firrtl.const.uint<2>) { From 71ed710900a7aaafb15b4dae2a3b242629d871d9 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Thu, 25 May 2023 11:06:58 -0600 Subject: [PATCH 10/11] Don't use recursion --- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 48 ++++++++++++++++---------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index b74020e94be5..d842148c6110 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -2354,30 +2354,6 @@ static bool isConstFieldDriven(FIRRTLBaseType type, bool isFlip = false, return !isFlip; return false; } - -/// Looks up the value's defining op until the defining op is null or a -/// declaration of the value. If a SubAccessOp is encountered with a 'const' -/// input, `originalFieldType` is made 'const'. -static Value -findFieldDeclarationRefiningFieldType(Value value, - FIRRTLBaseType &originalFieldType) { - auto *definingOp = value.getDefiningOp(); - if (!definingOp) - return value; - - return TypeSwitch(definingOp) - .Case([&](auto op) { - return findFieldDeclarationRefiningFieldType(op.getInput(), - originalFieldType); - }) - .Case([&](SubaccessOp op) { - if (op.getInput().getType().getElementTypePreservingConst().isConst()) - originalFieldType = originalFieldType.getConstType(true); - return findFieldDeclarationRefiningFieldType(op.getInput(), - originalFieldType); - }) - .Default([&](Operation *) { return value; }); -} // NOLINTEND(misc-no-recursion) /// Checks that connections to 'const' destinations are not dependent on @@ -2393,6 +2369,30 @@ static LogicalResult checkConnectConditionality(FConnectLike connect) { auto destRefinedType = destType; auto srcRefinedType = srcType; + /// Looks up the value's defining op until the defining op is null or a + /// declaration of the value. If a SubAccessOp is encountered with a 'const' + /// input, `originalFieldType` is made 'const'. + auto findFieldDeclarationRefiningFieldType = + [](Value value, FIRRTLBaseType &originalFieldType) -> Value { + while (auto *definingOp = value.getDefiningOp()) { + bool shouldContinue = true; + TypeSwitch(definingOp) + .Case([&](auto op) { value = op.getInput(); }) + .Case([&](SubaccessOp op) { + if (op.getInput() + .getType() + .getElementTypePreservingConst() + .isConst()) + originalFieldType = originalFieldType.getConstType(true); + value = op.getInput(); + }) + .Default([&](Operation *) { shouldContinue = false; }); + if (!shouldContinue) + break; + } + return value; + }; + auto destDeclaration = findFieldDeclarationRefiningFieldType(dest, destRefinedType); auto srcDeclaration = From d14e188fb1d877013e4fb510d007daa75e470136 Mon Sep 17 00:00:00 2001 From: Daniel Resnick Date: Thu, 25 May 2023 11:37:09 -0600 Subject: [PATCH 11/11] Fix const dropping subaccess handling to disallow any assignment --- lib/Dialect/FIRRTL/FIRRTLOps.cpp | 37 +++++++++++++++---------- test/Dialect/FIRRTL/connect-errors.mlir | 31 ++++++++++++++++++--- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/lib/Dialect/FIRRTL/FIRRTLOps.cpp b/lib/Dialect/FIRRTL/FIRRTLOps.cpp index d842148c6110..b6321b794fe9 100644 --- a/lib/Dialect/FIRRTL/FIRRTLOps.cpp +++ b/lib/Dialect/FIRRTL/FIRRTLOps.cpp @@ -2398,13 +2398,6 @@ static LogicalResult checkConnectConditionality(FConnectLike connect) { auto srcDeclaration = findFieldDeclarationRefiningFieldType(src, srcRefinedType); - // Make sure subaccesses don't allow for a non-'const' to 'const' connection - if ((destType != destRefinedType || srcType != srcRefinedType) && - !areTypesEquivalent(destRefinedType, srcRefinedType)) { - return connect.emitError("type mismatch between destination ") - << destRefinedType << " and source " << srcRefinedType; - } - auto checkConstConditionality = [&](Value value, FIRRTLBaseType type, Value declaration) -> LogicalResult { auto *declarationBlock = declaration.getParentBlock(); @@ -2428,16 +2421,30 @@ static LogicalResult checkConnectConditionality(FConnectLike connect) { return success(); }; + auto emitSubaccessError = [&] { + return connect.emitError( + "assignment to non-'const' subaccess of 'const' type is disallowed"); + }; + // Check destination if it contains 'const' leaves - if (destRefinedType.containsConst() && isConstFieldDriven(destRefinedType) && - failed(checkConstConditionality(dest, destType, destDeclaration))) - return failure(); + if (destRefinedType.containsConst() && isConstFieldDriven(destRefinedType)) { + // Disallow assignment to non-'const' subaccesses of 'const' types + if (destType != destRefinedType) + return emitSubaccessError(); + + if (failed(checkConstConditionality(dest, destType, destDeclaration))) + return failure(); + } // Check source if it contains 'const' 'flip' leaves if (srcRefinedType.containsConst() && - isConstFieldDriven(srcRefinedType, /*isFlip=*/true) && - failed(checkConstConditionality(src, srcType, srcDeclaration))) - return failure(); + isConstFieldDriven(srcRefinedType, /*isFlip=*/true)) { + // Disallow assignment to non-'const' subaccesses of 'const' types + if (srcType != srcRefinedType) + return emitSubaccessError(); + if (failed(checkConstConditionality(src, srcType, srcDeclaration))) + return failure(); + } return success(); } @@ -2797,8 +2804,8 @@ ParseResult ConstantOp::parse(OpAsmParser &parser, OperationState &result) { // top bits if it is a positive number. value = value.sext(width); } else if (width < value.getBitWidth()) { - // The parser can return an unnecessarily wide result with leading zeros. - // This isn't a problem, but truncating off bits is bad. + // The parser can return an unnecessarily wide result with leading + // zeros. This isn't a problem, but truncating off bits is bad. if (value.getNumSignBits() < value.getBitWidth() - width) return parser.emitError(loc, "constant too large for result type ") << resultType; diff --git a/test/Dialect/FIRRTL/connect-errors.mlir b/test/Dialect/FIRRTL/connect-errors.mlir index 086fd92bf925..72935acef433 100644 --- a/test/Dialect/FIRRTL/connect-errors.mlir +++ b/test/Dialect/FIRRTL/connect-errors.mlir @@ -611,27 +611,50 @@ firrtl.module @test(in %in : !firrtl.bundle>>, // ----- -// Test that subaccess of a const vector is checked as if the field is const. +// Test that non-const subaccess of a const vector disallows assignment. firrtl.circuit "test" { firrtl.module @test(in %index: !firrtl.uint<1>, out %out: !firrtl.const.vector, 1>) { %c = firrtl.constant 0 : !firrtl.uint<1> %d = firrtl.subaccess %out[%index] : !firrtl.const.vector, 1>, !firrtl.uint<1> - // expected-error @+1 {{type mismatch}} + // expected-error @+1 {{assignment to non-'const' subaccess of 'const' type is disallowed}} firrtl.strictconnect %d, %c : !firrtl.uint<1> } } // ----- -// Test that subaccess of a flipped const vector is checked as if the field is const. +// Test that non-const subaccess of a const vector disallows assignment, even if the source is const. +firrtl.circuit "test" { +firrtl.module @test(in %index: !firrtl.uint<1>, out %out: !firrtl.const.vector, 1>) { + %c = firrtl.constant 0 : !firrtl.const.uint<1> + %d = firrtl.subaccess %out[%index] : !firrtl.const.vector, 1>, !firrtl.uint<1> + // expected-error @+1 {{assignment to non-'const' subaccess of 'const' type is disallowed}} + firrtl.connect %d, %c : !firrtl.uint<1>, !firrtl.const.uint<1> +} +} + +// ----- + +// Test that non-const subaccess of a flipped const vector disallows assignment. firrtl.circuit "test" { firrtl.module @test(in %index: !firrtl.uint<1>, in %in: !firrtl.const.vector>, 1>, out %out: !firrtl.bundle>) { %element = firrtl.subaccess %in[%index] : !firrtl.const.vector>, 1>, !firrtl.uint<1> - // expected-error @+1 {{type mismatch}} + // expected-error @+1 {{assignment to non-'const' subaccess of 'const' type is disallowed}} firrtl.connect %out, %element : !firrtl.bundle>, !firrtl.bundle> } } +// ----- + +// Test that non-const subaccess of a flipped const vector disallows assignment, even if the source is const. +firrtl.circuit "test" { +firrtl.module @test(in %index: !firrtl.uint<1>, in %in: !firrtl.const.vector>, 1>, out %out: !firrtl.bundle>) { + %element = firrtl.subaccess %in[%index] : !firrtl.const.vector>, 1>, !firrtl.uint<1> + // expected-error @+1 {{assignment to non-'const' subaccess of 'const' type is disallowed}} + firrtl.connect %out, %element : !firrtl.bundle>, !firrtl.bundle> +} +} + // ----- firrtl.circuit "test" { firrtl.module @test(in %p: !firrtl.uint<1>, in %in: !firrtl.const.uint<2>, out %out: !firrtl.const.uint<2>) {