Skip to content

[FIRRTL] DomainFieldAttr types are PropertyTypes #9094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
seldridge merged 2 commits into from
Oct 15, 2025
Merged

[FIRRTL] DomainFieldAttr types are PropertyTypes #9094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ee9495
[FIRRTL] Split ODS Enums from Attributes
seldridge Oct 14, 2025
f47a6fe
[FIRRTL] DomainFieldAttr types are PropertyTypes
seldridge Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/circt/Dialect/FIRRTL/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ add_circt_dialect(FIRRTL firrtl FIRRTL)
set(LLVM_TARGET_DEFINITIONS FIRRTLEnums.td)
mlir_tablegen(FIRRTLEnums.h.inc -gen-enum-decls)
mlir_tablegen(FIRRTLEnums.cpp.inc -gen-enum-defs)
add_public_tablegen_target(CIRCTFIRRTLEnumsIncGen)

set(LLVM_TARGET_DEFINITIONS FIRRTL.td)
mlir_tablegen(FIRRTLAttributes.h.inc -gen-attrdef-decls -attrdefs-dialect=firrtl)
mlir_tablegen(FIRRTLAttributes.cpp.inc -gen-attrdef-defs -attrdefs-dialect=firrtl)
add_public_tablegen_target(CIRCTFIRRTLEnumsIncGen)
add_public_tablegen_target(CIRCTFIRRTLAttributesIncGen)

set(LLVM_TARGET_DEFINITIONS Passes.td)
mlir_tablegen(Passes.h.inc -gen-pass-decls)
Expand Down
53 changes: 2 additions & 51 deletions include/circt/Dialect/FIRRTL/FIRRTLAttributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,59 +14,10 @@
#define CIRCT_DIALECT_FIRRTL_FIRRTLATTRIBUTES_H

#include "circt/Dialect/FIRRTL/FIRRTLDialect.h"
#include "circt/Dialect/FIRRTL/FIRRTLEnums.h"
#include "circt/Dialect/FIRRTL/FIRRTLTypes.h"
#include "circt/Support/LLVM.h"

namespace circt {
namespace firrtl {

//===----------------------------------------------------------------------===//
// PortDirections
//===----------------------------------------------------------------------===//

/// This represents the direction of a single port.
enum class Direction { In, Out };

/// Prints the Direction to the stream as either "in" or "out".
llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const Direction &dir);

namespace direction {

/// Return an output direction if \p isOutput is true, otherwise return an
/// input direction.
static inline Direction get(bool isOutput) { return (Direction)isOutput; }

/// Convert from Direction to bool. The opposite of get;
static inline bool unGet(Direction dir) { return (bool)dir; }

/// Flip a port direction.
Direction flip(Direction direction);

static inline StringRef toString(Direction direction) {
return direction == Direction::In ? "in" : "out";
}

static inline StringRef toString(bool direction) {
return toString(get(direction));
}

/// Return a \p DenseBoolArrayAttr containing the packed representation of an
/// array of directions.
mlir::DenseBoolArrayAttr packAttribute(MLIRContext *context,
ArrayRef<Direction> directions);

/// Return a \p DenseBoolArrayAttr containing the packed representation of an
/// array of directions.
mlir::DenseBoolArrayAttr packAttribute(MLIRContext *context,
ArrayRef<bool> directions);

/// Turn a packed representation of port attributes into a vector that can
/// be worked with.
SmallVector<Direction> unpackAttribute(mlir::DenseBoolArrayAttr directions);

} // namespace direction
} // namespace firrtl
} // namespace circt

#define GET_ATTRDEF_CLASSES
#include "circt/Dialect/FIRRTL/FIRRTLAttributes.h.inc"

Expand Down
3 changes: 2 additions & 1 deletion include/circt/Dialect/FIRRTL/FIRRTLAttributes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#define CIRCT_DIALECT_FIRRTL_FIRRTLATTRIBUTES_TD

include "FIRRTLDialect.td"
include "FIRRTLTypes.td"
include "mlir/IR/BuiltinAttributeInterfaces.td"
include "circt/Types.td"

Expand Down Expand Up @@ -214,7 +215,7 @@ def DomainFieldAttr : AttrDef<FIRRTLDialect, "DomainField"> {
let summary = "A single field of a domain";
let parameters = (
ins "::mlir::StringAttr":$name,
"::mlir::TypeAttr":$type
"::circt::firrtl::PropertyType":$type
);
let assemblyFormat = [{
`<` $name `,` $type `>`
Expand Down
3 changes: 0 additions & 3 deletions include/circt/Dialect/FIRRTL/FIRRTLDialect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,4 @@
// Pull in the dialect definition.
#include "circt/Dialect/FIRRTL/FIRRTLDialect.h.inc"

// Pull in all enum type definitions and utility function declarations.
#include "circt/Dialect/FIRRTL/FIRRTLEnums.h.inc"
Copy link
Contributor

@dtzSiFive dtzSiFive Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was including this a problem?
Looking at upstream MLIR dialect headers, they seem to.. do things differently.

I think requiring including FIRRTLEnums.h separately is reasonable to match how we've organized attributes/operations/types similarly.

Good call, thanks!

Copy link
Member Author

@seldridge seldridge Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a problem if this was included by FIRRTLEnums.h (which the PR needed) as the .h.inc don't have any include guards. Some have the GET_<kind>_CLASSES guard which could help, but enums and attributes are totally unguarded. Hence, I needed to get this under something that we could include guard.

I was following along with how SPIRV dialect was defined upstream and modeling it off of that.

Yes, we are doing things differently from upstream and that makes me want to go redo our dialects to align with upstream...


#endif // CIRCT_DIALECT_FIRRTL_IR_DIALECT_H
73 changes: 73 additions & 0 deletions include/circt/Dialect/FIRRTL/FIRRTLEnums.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
//===- FIRRTLEnums.h - FIRRTL dialect enums ---------------------*- 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 contains custom FIRRTL Dialect enumerations.
//
//===----------------------------------------------------------------------===//

#ifndef CIRCT_DIALECT_FIRRTL_FIRRTLENUMS_H
#define CIRCT_DIALECT_FIRRTL_FIRRTLENUMS_H

#include "circt/Dialect/FIRRTL/FIRRTLDialect.h"
#include "circt/Support/LLVM.h"

namespace circt {
namespace firrtl {

//===----------------------------------------------------------------------===//
// PortDirections
//===----------------------------------------------------------------------===//

/// This represents the direction of a single port.
enum class Direction { In, Out };

/// Prints the Direction to the stream as either "in" or "out".
llvm::raw_ostream &operator<<(llvm::raw_ostream &os, const Direction &dir);

namespace direction {

/// Return an output direction if \p isOutput is true, otherwise return an
/// input direction.
static inline Direction get(bool isOutput) { return (Direction)isOutput; }

/// Convert from Direction to bool. The opposite of get;
static inline bool unGet(Direction dir) { return (bool)dir; }

/// Flip a port direction.
Direction flip(Direction direction);

static inline StringRef toString(Direction direction) {
return direction == Direction::In ? "in" : "out";
}

static inline StringRef toString(bool direction) {
return toString(get(direction));
}

/// Return a \p DenseBoolArrayAttr containing the packed representation of an
/// array of directions.
mlir::DenseBoolArrayAttr packAttribute(MLIRContext *context,
ArrayRef<Direction> directions);

/// Return a \p DenseBoolArrayAttr containing the packed representation of an
/// array of directions.
mlir::DenseBoolArrayAttr packAttribute(MLIRContext *context,
ArrayRef<bool> directions);

/// Turn a packed representation of port attributes into a vector that can
/// be worked with.
SmallVector<Direction> unpackAttribute(mlir::DenseBoolArrayAttr directions);

} // namespace direction
} // namespace firrtl
} // namespace circt

#define GET_ATTRDEF_CLASSES
#include "circt/Dialect/FIRRTL/FIRRTLEnums.h.inc"

#endif // CIRCT_DIALECT_FIRRTL_FIRRTLENUMS_H
2 changes: 2 additions & 0 deletions include/circt/Dialect/FIRRTL/FIRRTLOpInterfaces.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
#define CIRCT_DIALECT_FIRRTL_OP_INTERFACES_H

#include "circt/Dialect/FIRRTL/FIRRTLAnnotations.h"
#include "circt/Dialect/FIRRTL/FIRRTLAttributes.h"
#include "circt/Dialect/FIRRTL/FIRRTLEnums.h"
#include "circt/Dialect/FIRRTL/FIRRTLTypes.h"
#include "circt/Dialect/HW/HWAttributes.h"
#include "circt/Dialect/HW/HWOpInterfaces.h"
Expand Down
2 changes: 1 addition & 1 deletion include/circt/Dialect/FIRRTL/FIRRTLTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
#ifndef CIRCT_DIALECT_FIRRTL_TYPES_H
#define CIRCT_DIALECT_FIRRTL_TYPES_H

#include "circt/Dialect/FIRRTL/FIRRTLAttributes.h"
#include "circt/Dialect/FIRRTL/FIRRTLDialect.h"
#include "circt/Dialect/FIRRTL/FIRRTLEnums.h"
#include "circt/Dialect/FIRRTL/FIRRTLTypeInterfaces.h"
#include "circt/Dialect/HW/HWTypeInterfaces.h"
#include "circt/Support/LLVM.h"
Expand Down
2 changes: 1 addition & 1 deletion lib/Dialect/FIRRTL/Export/FIREmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ void Emitter::emitDeclaration(DomainOp op) {
for (auto attr : op.getFields()) {
auto fieldAttr = cast<DomainFieldAttr>(attr);
ps << PP::newline << PPExtString(fieldAttr.getName()) << " : ";
emitType(fieldAttr.getType().getValue());
emitType(fieldAttr.getType());
}
});
}
Expand Down
4 changes: 2 additions & 2 deletions lib/Dialect/FIRRTL/Import/FIRParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5813,8 +5813,8 @@ ParseResult FIRCircuitParser::parseDomain(CircuitOp circuit, unsigned indent) {
parsePropertyType(type, "field type") || info.parseOptionalInfo())
return failure();

fields.push_back(DomainFieldAttr::get(circuit.getContext(), fieldName,
TypeAttr::get(type)));
fields.push_back(
DomainFieldAttr::get(circuit.getContext(), fieldName, type));
}

auto builder = circuit.getBodyBuilder();
Expand Down
13 changes: 6 additions & 7 deletions lib/Dialect/FIRRTL/Transforms/LowerDomains.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,13 +495,12 @@ LogicalResult LowerCircuit::lowerDomain(DomainOp op) {
auto name = op.getNameAttr();
SmallVector<PortInfo> classInPorts;
for (auto field : op.getFields().getAsRange<DomainFieldAttr>())
classInPorts.append(
{{/*name=*/builder.getStringAttr(Twine(field.getName().getValue()) +
"_in"),
/*type=*/field.getType().getValue(), /*dir=*/Direction::In},
{/*name=*/builder.getStringAttr(Twine(field.getName().getValue()) +
"_out"),
/*type=*/field.getType().getValue(), /*dir=*/Direction::Out}});
classInPorts.append({{/*name=*/builder.getStringAttr(
Twine(field.getName().getValue()) + "_in"),
/*type=*/field.getType(), /*dir=*/Direction::In},
{/*name=*/builder.getStringAttr(
Twine(field.getName().getValue()) + "_out"),
/*type=*/field.getType(), /*dir=*/Direction::Out}});
auto classIn = ClassOp::create(builder, name, classInPorts);
auto classInType = classIn.getInstanceType();
auto pathListType =
Expand Down
9 changes: 9 additions & 0 deletions test/Dialect/FIRRTL/errors.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3151,3 +3151,12 @@ firrtl.circuit "Top" {
firrtl.domain.define %o, %i
}
}

// -----

// The type of a domain field must be a PropertyType.
firrtl.circuit "NonPropertyTypeInDomainField" {
// expected-error @below {{an array of domain fields}}
firrtl.domain @Foo ["bar", !firrtl.uint<1>]
firrtl.module @NonPropertyTypeInDomainField() {}
}