Skip to content
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

core: add replacement arg_values to inline_block_before #2061

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

superlopuh
Copy link
Member

Slightly modifies the semantics of inlining, bringing it in line with MLIR's behaviour, in my understanding. Currently, xDSL empties the block when inlining it to a different block, but does not erase it. With this PR, this behaviour is changed to erasing the block, potentially leaving an empty region.

@superlopuh superlopuh added the core xDSL core (ir, textual format, ...) label Jan 31, 2024
@superlopuh superlopuh self-assigned this Jan 31, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (9633775) 90.04% compared to head (d8011e0) 90.04%.
Report is 2 commits behind head on main.

Files Patch % Lines
tests/pattern_rewriter/test_pattern_rewriter.py 92.30% 0 Missing and 1 partial ⚠️
xdsl/rewriter.py 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2061   +/-   ##
=======================================
  Coverage   90.04%   90.04%           
=======================================
  Files         292      292           
  Lines       36436    36498   +62     
  Branches     5406     5415    +9     
=======================================
+ Hits        32807    32865   +58     
+ Misses       2858     2857    -1     
- Partials      771      776    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@compor
Copy link
Collaborator

compor commented Feb 1, 2024

Is this ready? I see commented code and TODOs.
Can you also please link where that behaviour occurs in MLIR? It saves a bit of time in regexing the codebase?

Thanks

@superlopuh
Copy link
Member Author

This is ready, there's just behaviour that we can currently not replicate due to lack of successor/predecessor accessors on Blocks in xDSL. I can add more comments to the commented code? I'll add the link to MLIR implementation.

@compor
Copy link
Collaborator

compor commented Feb 1, 2024

This is ready, there's just behaviour that we can currently not replicate due to lack of successor/predecessor accessors on Blocks in xDSL. I can add more comments to the commented code? I'll add the link to MLIR implementation.

OK, I'll have a look then, is it worth tracking the TODOs as issues?

Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I have a question about it.
Why do we want to check for the correctness of successors after inlining?
If we do multiple rewrites at once, I would expect to only verify the correctness of successors at the end of the sequence of rewrites?
Is it what MLIR does?

@superlopuh
Copy link
Member Author

Yeah that's fully lifted from the MLIR code base. I'm not 100% sure what all the reasons behind it are, but it feels reasonable for me to leave the commented stubs in so that we have something to go from when we hit a bug in a rewrite that leverages this.

@superlopuh superlopuh merged commit 15ecd0c into main Feb 1, 2024
10 checks passed
@superlopuh superlopuh deleted the sasha/core/inline-block-before branch February 1, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core xDSL core (ir, textual format, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants