[OM] Separates OM object fields verifier to a dedicated pass #7026
Conversation
Verification for OM object fields performs globaly analysis on a symbol table op. This cannot be done efficiently today within symbol table verfiication without nested symbol tables. So this PR separatse the verifier into a dedicated pass and run the verfication at several times in the pipeline.
|
|
||
| // The nested ClassOp must exist, since a field with ClassType | ||
| // must be an ObjectInstOp, which already verifies the class | ||
| // exists. |
There was a problem hiding this comment.
"me be an ObjectInstOp" -> that is, if this is valid and not an error, right?
If so, might be good to handle this either not finding a class or that the symbol resolves to a non-classlike. I think lookupNearestSymbolFrom<ClassLike>(..) might help with that.
There was a problem hiding this comment.
This might not be needed, just checking 👍 .
There was a problem hiding this comment.
Yes I think it could crash when object is not created by ObjectInstOp. I added a check and test. Thanks!
lib/Dialect/OM/OMOps.cpp
Outdated
| << getResult().getType() << ", but accessed field has type " | ||
| << finalField.getType(); | ||
|
|
||
| // This is verified by VerifyObjectFields pass. |
There was a problem hiding this comment.
Is there utility in implementing this / SymbolUserOpInterface if it is only ever success (and our symbol users probably don't work right anyway -- they aren't underneath the SymbolTable defining the symbols referenced AFAICT).
Anyway maybe fine for now, pending a rework esp now we're managing our own verification too.
There was a problem hiding this comment.
I think when we finally fix this for real, this will probably be where we re-land the verifier, since om.object.field will still reference the om.class symbol. The difference is we will re-work the IR to not have to then scan the om.class body to perform the verification. So I guess this is fine to rip out now, and we can add it back then, or we can leave the placeholder.
There was a problem hiding this comment.
I was thinking that since these aren't not compatible with MLIR symbols the rework wouldn't use this since these aren't symbol users in the normal sense?
But okay, something to chat about later...
There was a problem hiding this comment.
Yeah I think we can drop the interface. ObjectFieldOp would not be a MLIR symbol user.
uenoku
force-pushed
the
dev/hidetou/verifier
branch
from
545e536
to
f6efaa2
Compare
Verification for OM object fields performs global analysis on a symbol table op. This cannot be done efficiently today within symbol table verification without nested symbol tables. So this PR separates the verifier into a dedicated pass and run the verification at several times in the pipeline.