[OM] Rework Class and Object op to finish the move away from fields being symbols #7078
Labels
Comments
|
One thing @uenoku brought up is we will have cyclic references. So putting the array of field types into the |
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. (e.g. some call here https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 to emit "fields")
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. (e.g. some call here https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 to emit "fields")
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. (e.g. some call here https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 to emit "fields")
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. (e.g. some call here https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 to emit "fields")
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. (e.g. some call here https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 to emit "fields")
leonardt
added a commit
that referenced
this issue
Jul 12, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. E.g. some call here to "emit fields" https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 * Is there a way for `getFields` to have a type specified as an "iterator emitting type Field"? That is, the interface should be any range iterator over a collection of `Field` members (not necessarily the mapped iterator implementation used here). Hiding it behind a name helps enforce the interface, but if someone were to expect that mapped type, it would fail if we changed the underlying implementation to emit the fields differently. I think the appropriate type would be some `Iterator<Field>` type where any iterator producing a Field could be used. Exploring various ways to approach this using strategies such as iterator_traits etc... did not reveal anything useful yet
leonardt
added a commit
that referenced
this issue
Jul 23, 2024
The goal is to introduce an abstraction that allows us to change how fields are represented under the hood. This is a step towards addressing #7078 which aims to use a terminator op to declare and store the field name/type mapping. With this API in place, we can change how the underlying fields are accessed without having to update the producers/consumers of this information. Issues: * We introduce a `Field` struct to provide the information required by the evaluator. We could return the FieldOp instead since this just passes through the relevant data, but if we change to fetching this information from the terminator op instead, we'd need to update the consumer code, so introducing a new intermediate type abstracts how the information is stored/fetched (at the cost of introducing a layer of indirection). If we think this will be a performance issue, we may want to passthrough the underlying representation (currently FieldOp) and update the references when the internal implementation is changed. * The `addField` API will provide a hook to record field names and types for emitting the final terminator op, however is there a way for the `ClassOp` to run internal code at completion of the `lowerClass` logic? Otherwise, we will need to update the user code to run some kind of terminator function to tell the class to construct the final terminator op. E.g. some call here to "emit fields" https://github.com/llvm/circt/blob/e616cf1d327bd6748304f78755b034ba037224f0/lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp#L972 * Is there a way for `getFields` to have a type specified as an "iterator emitting type Field"? That is, the interface should be any range iterator over a collection of `Field` members (not necessarily the mapped iterator implementation used here). Hiding it behind a name helps enforce the interface, but if someone were to expect that mapped type, it would fail if we changed the underlying implementation to emit the fields differently. I think the appropriate type would be some `Iterator<Field>` type where any iterator producing a Field could be used. Exploring various ways to approach this using strategies such as iterator_traits etc... did not reveal anything useful yet
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Since the OM dialect's inception, fields were symbols in the Class. This was problematic, because Classes themselves are symbols in the builtin Module symbol table, so they can be referenced by Object ops and Object field ops.
As we have found time and again, nested symbol tables don't work, so we made fields no longer a symbol. But this meant we had to scan the class body while verifying fields. That became a performance problem, so we moved this logic out of a verifier, and instead ran a global Class/Object verification pass at certain points in the pipeline: #7026
This issue is to track the final resolution of this saga, which will probably encompass a few concrete steps.
In short, I think it makes sense for Class / Object to behave more like HWModule / Instance in the HW dialect. This will necessitate a few changes:
This is sufficient to get away from the half-baked sort of symbolic representation of om.field. But we need to address the core issue, which was verifier performance.
It might be possible to keep the om.class type as just a symbol reference to the om.class op with the above changes. The om.object op can verify the class exists and takes the specified inputs. The om.object.field op may be able to efficiently verify that the requested field exists with the correct type. One thing we can change is the om.object.field doesn't need to support a path of fields--it turned out we never needed this ability--it is only ever one field at a time.
If it would be helpful for verification performance, we could also consider augmenting the om.class type to include the fields, more like the hw.module_type. This would still need to be verified against the om.class, probably in the om.object verifier, but then the om.object.field verifier could check against the type directly.
The text was updated successfully, but these errors were encountered: