Skip to content

Commit c07d31b

Browse files
leonardtleonardt
authored
[OM] Add field_locs array attribute for ClassFieldsOp locations (#8439)
This changes the ClassFieldsOp logic to use an inherent attribute to store an array mapping field to location by index. This avoids the issue where FusedLoc merges non-unique locations (preventing us from retrieving a fields location by mapping it to an index). It also avoids an issue where metadata is removed from the FusedLoc that was previously being used to store this information.
1 parent b8e1446 commit c07d31b

File tree

6 files changed

+103
-38
lines changed

6 files changed

+103
-38
lines changed

‎include/circt/Dialect/OM/OMOps.td

+11-6
Original file line numberDiff line numberDiff line change
@@ -127,10 +127,9 @@ def ClassOp : OMClassLike<"class", [
127127
// the field location will break.
128128
// This is required because MLIR's FusedLoc uses a "set" semantics where a
129129
// single location is used to represent multiple fields with the same
130-
// location. The OM implementation uses the metadata attribute on FusedLoc
131-
// to store the original array of locations, so that the specific location
132-
// of a field may be easily retrieved by index using the
133-
// `getFieldLocByIndex` API.
130+
// location. The OM implementation uses an attribute to store the original
131+
// array of locations, so that the specific location of a field may be
132+
// easily retrieved by index using the `getFieldLocByIndex` API.
134133
void addNewFieldsOp(mlir::OpBuilder &builder, mlir::ArrayRef<mlir::Location>
135134
locs, mlir::ArrayRef<mlir::Value> values);
136135

@@ -156,8 +155,14 @@ def ClassOp : OMClassLike<"class", [
156155

157156
def ClassFieldsOp : OMOp<"class.fields", [Terminator, ReturnLike, Pure,
158157
HasParent<"ClassOp">]> {
159-
let arguments = (ins Variadic<AnyType>:$fields);
160-
let assemblyFormat = "attr-dict ($fields^ `:` qualified(type($fields)))?";
158+
let arguments = (ins Variadic<AnyType>:$fields,
159+
OptionalAttr<LocationArrayAttr>:$field_locs);
160+
let assemblyFormat = [{
161+
attr-dict ($fields^ `:` qualified(type($fields)))?
162+
custom<FieldLocs>($field_locs)
163+
}];
164+
165+
let hasVerifier = 1;
161166
}
162167

163168
//===----------------------------------------------------------------------===//

‎lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -1904,7 +1904,8 @@ struct ClassFieldsOpConversion : public OpConversionPattern<ClassFieldsOp> {
19041904
LogicalResult
19051905
matchAndRewrite(ClassFieldsOp op, OpAdaptor adaptor,
19061906
ConversionPatternRewriter &rewriter) const override {
1907-
rewriter.replaceOpWithNewOp<ClassFieldsOp>(op, adaptor.getOperands());
1907+
rewriter.replaceOpWithNewOp<ClassFieldsOp>(op, adaptor.getOperands(),
1908+
adaptor.getFieldLocsAttr());
19081909
return success();
19091910
}
19101911
};

‎lib/Dialect/OM/OMOps.cpp

+58-20
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ using namespace mlir;
2020
using namespace circt::om;
2121

2222
//===----------------------------------------------------------------------===//
23-
// Path Printers and Parsers
23+
// Custom Printers and Parsers
2424
//===----------------------------------------------------------------------===//
2525

2626
static ParseResult parseBasePathString(OpAsmParser &parser, PathAttr &path) {
@@ -74,6 +74,26 @@ static void printPathString(OpAsmPrinter &p, Operation *op, PathAttr path,
7474
p << '\"';
7575
}
7676

77+
static ParseResult parseFieldLocs(OpAsmParser &parser, ArrayAttr &fieldLocs) {
78+
if (parser.parseOptionalKeyword("field_locs"))
79+
return success();
80+
if (parser.parseLParen() || parser.parseAttribute(fieldLocs) ||
81+
parser.parseRParen()) {
82+
return failure();
83+
}
84+
return success();
85+
}
86+
87+
static void printFieldLocs(OpAsmPrinter &printer, Operation *op,
88+
ArrayAttr fieldLocs) {
89+
mlir::OpPrintingFlags flags;
90+
if (!flags.shouldPrintDebugInfo() || !fieldLocs)
91+
return;
92+
printer << "field_locs(";
93+
printer.printAttribute(fieldLocs);
94+
printer << ")";
95+
}
96+
7797
//===----------------------------------------------------------------------===//
7898
// Shared definitions
7999
//===----------------------------------------------------------------------===//
@@ -291,10 +311,16 @@ circt::om::ClassOp circt::om::ClassOp::buildSimpleClassOp(
291311
Block *body = &classOp.getRegion().emplaceBlock();
292312
auto prevLoc = odsBuilder.saveInsertionPoint();
293313
odsBuilder.setInsertionPointToEnd(body);
314+
315+
mlir::SmallVector<Attribute> locAttrs(fieldNames.size(), LocationAttr(loc));
316+
294317
odsBuilder.create<ClassFieldsOp>(
295-
loc, llvm::map_to_vector(fieldTypes, [&](Type type) -> Value {
296-
return body->addArgument(type, loc);
297-
}));
318+
loc,
319+
llvm::map_to_vector(
320+
fieldTypes,
321+
[&](Type type) -> Value { return body->addArgument(type, loc); }),
322+
odsBuilder.getArrayAttr(locAttrs));
323+
298324
odsBuilder.restoreInsertionPoint(prevLoc);
299325

300326
return classOp;
@@ -409,31 +435,25 @@ void circt::om::ClassOp::addNewFieldsOp(mlir::OpBuilder &builder,
409435
mlir::ArrayRef<Value> values) {
410436
// Store the original locations as a metadata array so that unique locations
411437
// are preserved as a mapping from field index to location
438+
assert(locs.size() == values.size() && "Expected a location per value");
412439
mlir::SmallVector<Attribute> locAttrs;
413440
for (auto loc : locs) {
414441
locAttrs.push_back(cast<Attribute>(LocationAttr(loc)));
415442
}
416443
// Also store the locations incase there's some other analysis that might
417444
// be able to use the default FusedLoc representation.
418-
builder.create<ClassFieldsOp>(
419-
builder.getFusedLoc(locs, builder.getArrayAttr(locAttrs)), values);
445+
builder.create<ClassFieldsOp>(builder.getFusedLoc(locs), values,
446+
builder.getArrayAttr(locAttrs));
420447
}
421448

422449
mlir::Location circt::om::ClassOp::getFieldLocByIndex(size_t i) {
423-
Location loc = this->getFieldsOp()->getLoc();
424-
if (auto locs = dyn_cast<FusedLoc>(loc)) {
425-
// Because it's possible for a user to construct a fields op directly and
426-
// place a FusedLoc that doersn't follow the storage format of
427-
// addNewFieldsOp, we assert the information has been stored appropriately
428-
ArrayAttr metadataArr = dyn_cast<ArrayAttr>(locs.getMetadata());
429-
assert(metadataArr && "Expected fused loc to store metadata array");
430-
assert(i < metadataArr.size() &&
431-
"expected index to be less than array size");
432-
LocationAttr locAttr = dyn_cast<LocationAttr>(metadataArr[i]);
433-
assert(locAttr && "expected metadataArr entry to be location attribute");
434-
loc = Location(locAttr);
435-
}
436-
return loc;
450+
auto fieldsOp = this->getFieldsOp();
451+
auto fieldLocs = fieldsOp.getFieldLocs();
452+
if (!fieldLocs.has_value())
453+
return fieldsOp.getLoc();
454+
assert(i < fieldLocs.value().size() &&
455+
"field index too large for location array");
456+
return cast<LocationAttr>(fieldLocs.value()[i]);
437457
}
438458

439459
//===----------------------------------------------------------------------===//
@@ -475,6 +495,24 @@ void circt::om::ClassExternOp::replaceFieldTypes(AttrTypeReplacer replacer) {
475495
replaceClassLikeFieldTypes(*this, replacer);
476496
}
477497

498+
//===----------------------------------------------------------------------===//
499+
// ClassFieldsOp
500+
//===----------------------------------------------------------------------===//
501+
//
502+
LogicalResult circt::om::ClassFieldsOp::verify() {
503+
auto fieldLocs = this->getFieldLocs();
504+
if (fieldLocs.has_value()) {
505+
auto fieldLocsVal = fieldLocs.value();
506+
if (fieldLocsVal.size() != this->getFields().size()) {
507+
auto error = this->emitOpError("size of field_locs (")
508+
<< fieldLocsVal.size()
509+
<< ") does not match number of fields ("
510+
<< this->getFields().size() << ")";
511+
}
512+
}
513+
return success();
514+
}
515+
478516
//===----------------------------------------------------------------------===//
479517
// ObjectOp
480518
//===----------------------------------------------------------------------===//

‎test/Dialect/OM/errors.mlir

+16
Original file line numberDiff line numberDiff line change
@@ -178,3 +178,19 @@ om.class @A(%arg: i1) -> (a: i2) {
178178
// expected-note @+1 {{see terminator:}}
179179
om.class.fields %arg : i1
180180
}
181+
182+
183+
// -----
184+
185+
om.class @A(%arg: i1) -> (a: i1) {
186+
// expected-error @+1 {{expected ')'}}
187+
om.class.fields %arg : i1 field_locs([loc("loc0")], [loc("loc1")])
188+
}
189+
190+
191+
// -----
192+
193+
om.class @A(%arg: i1) -> (a: i1) {
194+
// expected-error @+1 {{'om.class.fields' op size of field_locs (2) does not match number of fields (1)}}
195+
om.class.fields %arg : i1 field_locs([loc("loc0"), loc("loc1")])
196+
}

‎test/Dialect/OM/round-trip.mlir

+2-2
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ om.class @Thingy(%blue_1: i8, %blue_2: i32) -> (widget: !om.class.type<@Widget>,
2020
// CHECK: %[[widget_field:.+]] = om.object.field %[[widget]], [@blue_1] : (!om.class.type<@Widget>) -> i8
2121
%6 = om.object.field %2, [@blue_1] : (!om.class.type<@Widget>) -> i8
2222

23-
// CHECK: om.class.fields {test = "fieldsAttr"} %2, %5, %blue_1, %6 : !om.class.type<@Widget>, !om.class.type<@Gadget>, i8, i8 loc("test")
24-
om.class.fields {test = "fieldsAttr"} %2, %5, %blue_1, %6 : !om.class.type<@Widget>, !om.class.type<@Gadget>, i8, i8 loc("test")
23+
// CHECK: om.class.fields {test = "fieldsAttr"} %2, %5, %blue_1, %6 : !om.class.type<@Widget>, !om.class.type<@Gadget>, i8, i8 field_locs([loc("loc0"), loc("loc1"), loc("loc2"), loc("loc3")]) loc("test")
24+
om.class.fields {test = "fieldsAttr"} %2, %5, %blue_1, %6 : !om.class.type<@Widget>, !om.class.type<@Gadget>, i8, i8 field_locs([loc("loc0"), loc("loc1"), loc("loc2"), loc("loc3")]) loc("test")
2525
}
2626

2727
// CHECK-LABEL: om.class @Widget

‎unittests/Dialect/OM/Evaluator/EvaluatorTests.cpp

+14-9
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,8 @@ TEST(EvaluatorTests, GetFieldInvalidName) {
168168
auto cls = builder.create<ClassOp>("MyClass");
169169
auto &body = cls.getBody().emplaceBlock();
170170
builder.setInsertionPointToStart(&body);
171-
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>());
171+
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>(),
172+
ArrayAttr{});
172173

173174
Evaluator evaluator(mod);
174175

@@ -253,7 +254,8 @@ TEST(EvaluatorTests, InstantiateObjectWithConstantField) {
253254
builder.setInsertionPointToStart(&body);
254255
auto constant = builder.create<ConstantOp>(
255256
circt::om::IntegerAttr::get(&context, constantType));
256-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({constant}));
257+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({constant}),
258+
ArrayAttr{});
257259

258260
Evaluator evaluator(mod);
259261

@@ -304,7 +306,7 @@ TEST(EvaluatorTests, InstantiateObjectWithChildObject) {
304306
body.addArgument(circt::om::OMIntegerType::get(&context), cls.getLoc());
305307
builder.setInsertionPointToStart(&body);
306308
auto object = builder.create<ObjectOp>(innerCls, body.getArguments());
307-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object}));
309+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object}), ArrayAttr{});
308310

309311
Evaluator evaluator(mod);
310312

@@ -368,7 +370,7 @@ TEST(EvaluatorTests, InstantiateObjectWithFieldAccess) {
368370
builder.create<ObjectFieldOp>(builder.getI32Type(), object,
369371
builder.getArrayAttr(FlatSymbolRefAttr::get(
370372
builder.getStringAttr("field"))));
371-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({field}));
373+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({field}), ArrayAttr{});
372374

373375
Evaluator evaluator(mod);
374376

@@ -407,7 +409,8 @@ TEST(EvaluatorTests, InstantiateObjectWithChildObjectMemoized) {
407409
auto innerCls = builder.create<ClassOp>("MyInnerClass");
408410
auto &innerBody = innerCls.getBody().emplaceBlock();
409411
builder.setInsertionPointToStart(&innerBody);
410-
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>());
412+
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>(),
413+
ArrayAttr{});
411414

412415
builder.setInsertionPointToStart(&mod.getBodyRegion().front());
413416
auto innerType = TypeAttr::get(ClassType::get(
@@ -422,7 +425,8 @@ TEST(EvaluatorTests, InstantiateObjectWithChildObjectMemoized) {
422425
auto &body = cls.getBody().emplaceBlock();
423426
builder.setInsertionPointToStart(&body);
424427
auto object = builder.create<ObjectOp>(innerCls, body.getArguments());
425-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object, object}));
428+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object, object}),
429+
ArrayAttr{});
426430

427431
Evaluator evaluator(mod);
428432

@@ -476,7 +480,8 @@ TEST(EvaluatorTests, AnyCastObject) {
476480
auto innerCls = builder.create<ClassOp>("MyInnerClass");
477481
auto &innerBody = innerCls.getBody().emplaceBlock();
478482
builder.setInsertionPointToStart(&innerBody);
479-
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>());
483+
builder.create<ClassFieldsOp>(loc, llvm::ArrayRef<mlir::Value>(),
484+
ArrayAttr{});
480485

481486
builder.setInsertionPointToStart(&mod.getBodyRegion().front());
482487
auto innerType = TypeAttr::get(ClassType::get(
@@ -491,7 +496,7 @@ TEST(EvaluatorTests, AnyCastObject) {
491496
builder.setInsertionPointToStart(&body);
492497
auto object = builder.create<ObjectOp>(innerCls, body.getArguments());
493498
auto cast = builder.create<AnyCastOp>(object);
494-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({cast}));
499+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({cast}), ArrayAttr{});
495500

496501
Evaluator evaluator(mod);
497502

@@ -545,7 +550,7 @@ TEST(EvaluatorTests, AnyCastParam) {
545550
auto cast = builder.create<AnyCastOp>(body.getArgument(0));
546551
SmallVector<Value> objectParams = {cast};
547552
auto object = builder.create<ObjectOp>(innerCls, objectParams);
548-
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object}));
553+
builder.create<ClassFieldsOp>(loc, SmallVector<Value>({object}), ArrayAttr{});
549554

550555
Evaluator evaluator(mod);
551556

0 commit comments

Comments
 (0)