[OM] Clean up ObjectOfieldOp: Move away from symbols, remove a field path and VerifyObjectFields pass#10303
Merged
Merged
Conversation
uenoku
force-pushed
the
dev/hidetou/om-clean-up
branch
from
e0237fd to
7ced68b
Compare
uenoku
commented
Apr 23, 2026
| %0 = om.object @ListCreateTest(%notpath, %basepath, %path) : (i1, !om.basepath, !om.path) -> !om.class.type<@ListCreateTest> | ||
|
|
||
| // CHECK: [[SUBFIELD:%.+]] = om.object.field [[OBJ]], [@nestedpath] : (!om.class.type<@PathTest>) -> !om.list<!om.list<!om.frozenpath>> | ||
| %1 = om.object.field %0, [@nestedpath] : (!om.class.type<@PathTest>) -> !om.list<!om.list<!om.path>> |
Member
Author
There was a problem hiding this comment.
This test was updated because @nestedath didn't exist in @PathTest.
uenoku
force-pushed
the
dev/hidetou/om-clean-up
branch
from
7ced68b to
9879c60
Compare
uenoku
commented
Apr 23, 2026
| } | ||
| firrtl.module private @WillBeReplaced(out %output: !firrtl.integer) { | ||
| %c = firrtl.integer 42 | ||
| firrtl.propassign %output, %c : !firrtl.integer |
Member
Author
There was a problem hiding this comment.
This was added since otherwise it causes verification error after lowering.
uenoku
commented
Apr 23, 2026
| let arguments = (ins | ||
| ClassType:$object, | ||
| FlatSymbolRefArrayAttr:$fieldPath | ||
| StrAttr:$field |
Member
Author
There was a problem hiding this comment.
I considered to use an index instead of StringAttr in a similar way to HWStructExtractOp, but I didn't do that because OM class type doesn't contain field types (so it's not possible to lookup field type anyway).
uenoku
force-pushed
the
dev/hidetou/om-clean-up
branch
2 times, most recently
from
4cce16a to
97ba9f0
Compare
Replace FlatSymbolRefArrayAttr with StringAttr for field names, changing syntax from 'object, [@field]' to 'object["field"]'. Nested field access now requires chaining ObjectFieldOp instead of using an array path. This moves verification from the VerifyObjectFields pass into the ObjectFieldOp verifier itself by adding SymbolUserOpInterface, eliminating the need for a separate pass. The Evaluator is updated to handle StringAttr and dereference ReferenceValue.
uenoku
force-pushed
the
dev/hidetou/om-clean-up
branch
from
97ba9f0 to
22db8d5
Compare
mikeurbach
approved these changes
Apr 28, 2026
Contributor
mikeurbach
left a comment
mikeurbach
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for wrapping up this longstanding tech debt. @leonardt please take a look as well, since this is building off the work you did a long time back.
Member
Author
|
Please let me merge this to unblock dependent work. @leonardt I'd appreciate post commit review! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR contains three refactoring/fix for ObjectOfieldOp. It may be better to split the PR but they are fairly coupled to each other so I found it's simpler to do it in a one PR.
Replace FlatSymbolRefArrayAttr with StringAttr for field names, changing syntax from
object, [@field]toobject["field"]. Nested field access now requires chaining ObjectFieldOp instead of using an array path for the simplicity. FIRRTL has only used single field access, so it doesn't change anything for firtool pipeline.This replaces VerifyObjectFields pass with the ObjectFieldOp verifier itself by adding SymbolUserOpInterface.
lower-classes.mlirtest is updated since it created an IR that doesn't pass verifier. This will increase the verifier runtime for core dialects since we now verify symbol uses of object field op in a verifier for good or bad, so it may be controversialIt also fixes Evaluator bug that didn't dereference ReferenceValue of object value as exposed by refactoring 1.
Close #7078. My primary motivation was to remove nested field access, since it makes IR mutation approach mentioned in #10265 (comment) way more simpler to implement.
Assisted-by: Augment: claude-sonnet-4.5