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 BlockInsertPoint to simplify the builder API #3703

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

math-fehr
Copy link
Collaborator

@math-fehr math-fehr commented Jan 6, 2025

Stacked PRs:


core: Add BlockInsertPoint to simplify the builder API

BlockInsertPoint acts the same as InsertPoint, but for blocks.
Its equivalent in MLIR is Block::iterator.

@math-fehr math-fehr changed the base branch from math-fehr/stack/6 to main January 6, 2025 15:47
@math-fehr math-fehr changed the base branch from main to math-fehr/stack/6 January 6, 2025 15:47
@math-fehr math-fehr self-assigned this Jan 6, 2025
@math-fehr math-fehr added the core xDSL core (ir, textual format, ...) label Jan 6, 2025
math-fehr added a commit that referenced this pull request Jan 6, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
@math-fehr math-fehr changed the base branch from math-fehr/stack/6 to main January 6, 2025 15:50
@math-fehr math-fehr changed the base branch from main to math-fehr/stack/6 January 6, 2025 15:50
math-fehr added a commit that referenced this pull request Jan 6, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
@math-fehr math-fehr changed the base branch from math-fehr/stack/6 to main January 6, 2025 15:52
@math-fehr math-fehr changed the base branch from main to math-fehr/stack/6 January 6, 2025 15:53
@alexarice
Copy link
Collaborator

Could InsertPoint and BlockInsertPoint be a generic class instead? They superficially look very similar except for some types.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.25%. Comparing base (6b2b23c) to head (84b071f).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3703      +/-   ##
==========================================
+ Coverage   91.23%   91.25%   +0.01%     
==========================================
  Files         459      459              
  Lines       57307    57351      +44     
  Branches     5532     5533       +1     
==========================================
+ Hits        52286    52333      +47     
+ Misses       3594     3593       -1     
+ Partials     1427     1425       -2     

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

@compor
Copy link
Collaborator

compor commented Jan 6, 2025

LGTM modulo the missing tests.

math-fehr added a commit that referenced this pull request Jan 6, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
Base automatically changed from math-fehr/stack/6 to main January 6, 2025 17:47
math-fehr added a commit that referenced this pull request Jan 6, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
math-fehr added a commit that referenced this pull request Jan 6, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
@math-fehr
Copy link
Collaborator Author

Could InsertPoint and BlockInsertPoint be a generic class instead? They superficially look very similar except for some types.

I can make them generic, I am just a bit scared of what the documentation will look like.
As you want, I can make it generic now, or later as well

@alexarice
Copy link
Collaborator

Maybe just leave it as is. The documentation is important

Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Couple of points that don't need to be dealt with in this PR:

  • Should InsertPoint be renamed to OpInsertPoint?
  • Is it possible/preferable to make the initializer private and force the use of the static methods to construct both InsertPoints and BlockInsertPoints, letting the runtime check be dropped?

math-fehr added a commit that referenced this pull request Jan 13, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
math-fehr added a commit that referenced this pull request Jan 20, 2025
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
`BlockInsertPoint` acts the same as `InsertPoint`, but for blocks.
Its equivalent in MLIR is `Block::iterator`.

stack-info: PR: #3703, branch: math-fehr/stack/7
@math-fehr
Copy link
Collaborator Author

Should InsertPoint be renamed to OpInsertPoint?

Yes, I think we could rename InsertPoint to OpInsertPoint later on (even if MLIR names it InsertPoint).

Is it possible/preferable to make the initializer private and force the use of the static methods to construct both InsertPoints and BlockInsertPoints, letting the runtime check be dropped?

I never found an actual way to make Python constructors private, which otherwise yes that would make things much better.

@math-fehr math-fehr merged commit bc48799 into main Jan 20, 2025
16 checks passed
@math-fehr math-fehr deleted the math-fehr/stack/7 branch January 20, 2025 05:07
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.

4 participants