Implement is_optional_symbol for MLIR compliance. #1330
Merged
Conversation
This file contains 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
Implement OptionalSymbolOpInterface for user-friendliness. Test it.
… symbol table work with the MLIR-compliant implementation.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1330 +/- ##
==========================================
- Coverage 89.64% 89.64% -0.01%
==========================================
Files 191 191
Lines 24868 24898 +30
Branches 3750 3756 +6
==========================================
+ Hits 22294 22320 +26
- Misses 1982 1984 +2
- Partials 592 594 +2
☔ View full report in Codecov by Sentry. |
PapyChacal
commented
Jul 24, 2023
xdsl/traits.py
Outdated
Comment on lines
256
to
265
| class OptionalSymbolOpInterface(SymbolOpInterface): | ||
| """ | ||
| Helper interface specialization for an optional Symbol. | ||
| """ | ||
|
|
||
| @staticmethod | ||
| def is_optional_symbol(op: Operation) -> bool: | ||
| return True | ||
|
|
||
|
|
There was a problem hiding this comment.
Digging in MLIR's sources, that's the only specialization I saw, and a few times, hence exposing it directly as is in the xDSL practical/firendly mindset!
compor
approved these changes
Jul 24, 2023
Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>
Co-authored-by: Chris Vasiladiotis <cvassiladiotis@gmail.com>
math-fehr
reviewed
Jul 24, 2023
math-fehr
reviewed
Jul 24, 2023
Co-authored-by: Fehr Mathieu <mathieu.fehr@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
That is, the
is_optional_symbolmethod ofSymbolOpInterface, matching MLIR'sisOptionalSymbolonSymbolImplement OptionalSymbolOpInterface for user-friendliness. Test it.
Our current implementation of SymbolOps is not compliant with MLIR's, which allows optionally symbol operations 🙃
The most famous example being
builtin.module, which can be anonymous as often for the top-level module, thus not containing anysym_nameattribute.I also take the opprtunity to make
builtin.modulean optional symbol operation. With this MLIR-compliant implementation, the only impact is to make the interpreter work with this compliance 🙂