Skip to content

Commit

Permalink
[FIRRTL] Fix unpacked array ordering in GCT and ExportVerilog (llvm#2049
Browse files Browse the repository at this point in the history 
)

The FIRRTL Grand Central pass and ExportVerilog currently order unpacked
arrays in reverse upon emission. A nested array like
`uarray<2 x uarray<3 x i1>>` has the shape `[[a,b,c], [d,e,f]]` and
should produce the following verilog:

  logic foo [2][3]; // non-range indices used for clarity

Generally the following SV declaration:

  logic [2:0][1:0] bar [4:0][3:0];

Reads its dimensions from outer- to innermost as:

  [4:0], [3:0], [2:0], [1:0]

Currently, ExportVerilog will emit the above `foo` example as
`foo[3][2]`, which is incorrect. Fixing ExportVerilog also provides a
fix for an issue @seldridge tackled earlier in ca5bfac. This change
reverts parts of that older fix since the updated declaration order now
makes the original Grand Central index order correct.

The output now matches what the Scala FIRRTL compiler produces.
  • Loading branch information
@fabianschuiki
fabianschuiki committed Oct 28, 2021
1 parent 67bc21a commit 027e6ef
Show file tree
Hide file tree
Showing 6 changed files with 345 additions and 39 deletions.
2 changes: 1 addition & 1 deletion lib/Conversion/ExportVerilog/ExportVerilog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -933,8 +933,8 @@ void ModuleEmitter::printUnpackedTypePostfix(Type type, raw_ostream &os) {
printUnpackedTypePostfix(inoutType.getElementType(), os);
})
.Case<UnpackedArrayType>([&](UnpackedArrayType arrayType) {
printUnpackedTypePostfix(arrayType.getElementType(), os);
os << "[0:" << (arrayType.getSize() - 1) << "]";
printUnpackedTypePostfix(arrayType.getElementType(), os);
})
.Case<InterfaceType>([&](auto) {
// Interface instantiations have parentheses like a module with no
Expand Down
50 changes: 23 additions & 27 deletions lib/Dialect/FIRRTL/Transforms/GrandCentral.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ struct VerbatimType {
SmallString<64> stringType(str);
stringType.append(" ");
stringType.append(name);
for (auto d : dimensions) {
for (auto d : llvm::reverse(dimensions)) {
stringType.append("[");
stringType.append(Twine(d).str());
stringType.append("]");
Expand Down Expand Up @@ -139,20 +139,20 @@ struct GrandCentralPass : public GrandCentralBase<GrandCentralPass> {

/// Recursively examine an AugmentedType to populate the "mappings" file
/// (generate XMRs) for this interface. This does not build new interfaces.
bool traverseField(Attribute field, IntegerAttr id, Twine path, Twine index);
bool traverseField(Attribute field, IntegerAttr id, Twine path);

/// Recursively examine an AugmentedType to both build new interfaces and
/// populate a "mappings" file (generate XMRs) using `traverseField`. Return
/// the type of the field exmained.
Optional<TypeSum> computeField(Attribute field, IntegerAttr id,
StringAttr prefix, Twine path, Twine index);
StringAttr prefix, Twine path);

/// Recursively examine an AugmentedBundleType to both build new interfaces
/// and populate a "mappings" file (generate XMRs). Return none if the
/// interface is invalid.
Optional<sv::InterfaceOp> traverseBundle(AugmentedBundleTypeAttr bundle,
IntegerAttr id, StringAttr prefix,
Twine path, Twine index);
Twine path);

/// Return the module associated with this value.
FModuleLike getEnclosingModule(Value value);
Expand Down Expand Up @@ -295,7 +295,7 @@ Optional<Attribute> GrandCentralPass::fromAttr(Attribute attr) {
}

bool GrandCentralPass::traverseField(Attribute field, IntegerAttr id,
Twine path, Twine index) {
Twine path) {
return TypeSwitch<Attribute, bool>(field)
.Case<AugmentedGroundTypeAttr>([&](auto ground) {
Value leafValue = leafMap.lookup(ground.getID());
Expand All @@ -319,16 +319,15 @@ bool GrandCentralPass::traverseField(Attribute field, IntegerAttr id,
FModuleOp module =
cast<FModuleOp>(blockArg.getOwner()->getParentOp());
builder.create<sv::VerbatimOp>(
uloc, "assign " + path + index + " = " + srcRef +
uloc, "assign " + path + " = " + srcRef +
module.getPortName(blockArg.getArgNumber()) + ";");
} else {
auto leafModuleName = leafValue.getDefiningOp()
->getAttr("name")
.cast<StringAttr>()
.getValue();
builder.create<sv::VerbatimOp>(uloc, "assign " + path + index +
" = " + srcRef +
leafModuleName + ";");
builder.create<sv::VerbatimOp>(
uloc, "assign " + path + " = " + srcRef + leafModuleName + ";");
}
return true;
})
Expand All @@ -339,8 +338,8 @@ bool GrandCentralPass::traverseField(Attribute field, IntegerAttr id,
auto field = fromAttr(elements[i]);
if (!field)
return false;
notFailed &= traverseField(field.getValue(), id, path,
+"[" + Twine(i) + "]" + index);
notFailed &=
traverseField(field.getValue(), id, path + "[" + Twine(i) + "]");
}
return notFailed;
})
Expand All @@ -354,8 +353,7 @@ bool GrandCentralPass::traverseField(Attribute field, IntegerAttr id,
if (!name)
name = element.cast<DictionaryAttr>().getAs<StringAttr>("defName");
anyFailed &=
traverseField(field.getValue(), id,
path + index + "." + name.getValue(), Twine());
traverseField(field.getValue(), id, path + "." + name.getValue());
}

return anyFailed;
Expand All @@ -371,8 +369,8 @@ bool GrandCentralPass::traverseField(Attribute field, IntegerAttr id,

Optional<TypeSum> GrandCentralPass::computeField(Attribute field,
IntegerAttr id,
StringAttr prefix, Twine path,
Twine index) {
StringAttr prefix,
Twine path) {

auto unsupported = [&](StringRef name, StringRef kind) {
return VerbatimType({("// <unsupported " + kind + " type>").str(), false});
Expand All @@ -382,7 +380,7 @@ Optional<TypeSum> GrandCentralPass::computeField(Attribute field,
.Case<AugmentedGroundTypeAttr>(
[&](AugmentedGroundTypeAttr ground) -> Optional<TypeSum> {
// Traverse to generate mappings.
traverseField(field, id, path, index);
traverseField(field, id, path);
auto value = leafMap.lookup(ground.getID());
auto tpe = value.getType().cast<FIRRTLType>();
if (!tpe.isGround()) {
Expand All @@ -401,16 +399,16 @@ Optional<TypeSum> GrandCentralPass::computeField(Attribute field,
auto elements = vector.getElements();
auto firstElement = fromAttr(elements[0]);
auto elementType = computeField(firstElement.getValue(), id, prefix,
path, "[0]" + index);
path + "[" + Twine(0) + "]");
if (!elementType)
return None;

for (size_t i = 1, e = elements.size(); i != e; ++i) {
auto subField = fromAttr(elements[i]);
if (!subField)
return None;
notFailed &= traverseField(subField.getValue(), id, path,
"[" + Twine(i) + "]" + index);
notFailed &= traverseField(subField.getValue(), id,
path + "[" + Twine(i) + "]");
}

if (auto *tpe = std::get_if<Type>(&elementType.getValue()))
Expand All @@ -422,7 +420,7 @@ Optional<TypeSum> GrandCentralPass::computeField(Attribute field,
})
.Case<AugmentedBundleTypeAttr>(
[&](AugmentedBundleTypeAttr bundle) -> TypeSum {
auto iface = traverseBundle(bundle, id, prefix, path, index);
auto iface = traverseBundle(bundle, id, prefix, path);
assert(iface && iface.getValue());
return VerbatimType({getInterfaceName(prefix, bundle), true});
})
Expand Down Expand Up @@ -454,7 +452,7 @@ Optional<TypeSum> GrandCentralPass::computeField(Attribute field,
/// drive the interface. Returns false on any failure and true on success.
Optional<sv::InterfaceOp>
GrandCentralPass::traverseBundle(AugmentedBundleTypeAttr bundle, IntegerAttr id,
StringAttr prefix, Twine path, Twine index) {
StringAttr prefix, Twine path) {
auto builder = OpBuilder::atBlockEnd(getOperation().getBody());
sv::InterfaceOp iface;
builder.setInsertionPointToEnd(getOperation().getBody());
Expand All @@ -476,9 +474,8 @@ GrandCentralPass::traverseBundle(AugmentedBundleTypeAttr bundle, IntegerAttr id,
return None;

auto name = element.cast<DictionaryAttr>().getAs<StringAttr>("name");
auto elementType =
computeField(field.getValue(), id, prefix,
path + index + "." + name.getValue(), Twine());
auto elementType = computeField(field.getValue(), id, prefix,
path + "." + name.getValue());
if (!elementType)
return None;

Expand Down Expand Up @@ -954,9 +951,8 @@ void GrandCentralPass::runOnOperation() {
// Error out if this returns None (indicating that the annotation annotation
// is malformed in some way). A good error message is generated inside
// `traverseBundle` or the functions it calls.
auto iface =
traverseBundle(bundle, bundle.getID(), bundle.getPrefix(),
companionIDMap.lookup(bundle.getID()).name, Twine());
auto iface = traverseBundle(bundle, bundle.getID(), bundle.getPrefix(),
companionIDMap.lookup(bundle.getID()).name);
if (!iface) {
removalError = true;
continue;
Expand Down
4 changes: 2 additions & 2 deletions test/Conversion/ExportVerilog/hw-dialect.mlir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,8 @@ hw.module @wires(%in4: i4, %in8: i8) -> (a: i4, b: i8, c: i8) {
// CHECK-NEXT: wire [7:0] myUArray1[0:41];
%myUArray1 = sv.wire : !hw.inout<uarray<42 x i8>>

// CHECK-NEXT: wire [41:0][3:0] myWireUArray2[0:2];
%myWireUArray2 = sv.wire : !hw.inout<uarray<3 x array<42 x i4>>>
// CHECK-NEXT: wire [9:0][7:0] myUArray2[0:13][0:11];
%myUArray2 = sv.wire : !hw.inout<uarray<14 x uarray<12 x array<10 x i8>>>>

// CHECK-EMPTY:

Expand Down
121 changes: 121 additions & 0 deletions test/Dialect/FIRRTL/SFCTests/GrandCentralInterfaces/Wire.anno.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,127 @@
]
}
},
{
"name": "multivec",
"description": "a 2D vector called 'multivec'",
"tpe": {
"class": "sifive.enterprise.grandcentral.AugmentedVectorType",
"elements": [
{
"class": "sifive.enterprise.grandcentral.AugmentedVectorType",
"elements": [
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 0},
{"class": "firrtl.annotations.TargetToken$Index", "value": 0}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
},
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 0},
{"class": "firrtl.annotations.TargetToken$Index", "value": 1}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
},
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 0},
{"class": "firrtl.annotations.TargetToken$Index", "value": 2}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
}
]
},
{
"class": "sifive.enterprise.grandcentral.AugmentedVectorType",
"elements": [
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 1},
{"class": "firrtl.annotations.TargetToken$Index", "value": 0}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
},
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 1},
{"class": "firrtl.annotations.TargetToken$Index", "value": 1}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
},
{
"class": "sifive.enterprise.grandcentral.AugmentedGroundType",
"ref": {
"circuit": "Top",
"module": "DUT",
"path": [],
"ref": "w",
"component": [
{"class": "firrtl.annotations.TargetToken$Field", "value": "multivec"},
{"class": "firrtl.annotations.TargetToken$Index", "value": 1},
{"class": "firrtl.annotations.TargetToken$Index", "value": 2}
]
},
"tpe": {
"class": "sifive.enterprise.grandcentral.GrandCentralView$UnknownGroundType$"
}
}
]
}
]
}
},
{
"name": "vecOfBundle",
"description": "a vector of a bundle",
Expand Down
Loading

0 comments on commit 027e6ef

Please sign in to comment.