-
Notifications
You must be signed in to change notification settings - Fork 299
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.
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
[OM] Add location info to EvaluatorValue #6240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't been working with this code a lot and not expert re:python bindings aspect, but generally LGTM! Small nit re:test.
16e4084
to
8f66b61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The evaluator logic for fields looks good, thanks. I still have one comment about the location for objects, as well as some API design thoughts about the Python side.
8f66b61
to
1916916
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still getting familiar with the latest refactor here to support graph regions, but I think this makes sense. Thanks for the iterations on this and dealing with the big refactor while this PR is in flight!
I would appreciate a second pair of eyes.. @uenoku do you mind taking a look?
# CHECK: name: reference, field: ('Root', 'x') | ||
print(f"name: {name}, field: {field}") | ||
# CHECK-SAME: loc: loc("-":32:7) | ||
#location from om.class.field @field, %param : !om.integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extreme nit:
#location from om.class.field @field, %param : !om.integer | |
# location from om.class.field @field, %param : !om.integer |
: EvaluatorValue(attr.getContext(), Kind::Attr, | ||
mlir::UnknownLoc::get(attr.getContext())), | ||
attr(attr) { | ||
markFullyEvaluated(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse AttributeValue(Attribute attr, Location loc)
constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for working on this!
Add the debug locations to the evaluator value, which will be used to generate
info
fields of the object model output json.