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

dialects: (affine) use maps in affine.for bounds, and add some helpers #1209

Merged
merged 2 commits into from Jul 3, 2023

Conversation

superlopuh
Copy link
Member

@superlopuh superlopuh added the dialects Changes on the dialects label Jun 27, 2023
@superlopuh superlopuh self-assigned this Jun 27, 2023
@superlopuh superlopuh changed the base branch from main to sasha/affine/for-results June 27, 2023 17:46
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 61.90% and project coverage change: -0.06 ⚠️

Comparison is base (fba9b00) 88.66% compared to head (8d74060) 88.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1209      +/-   ##
==========================================
- Coverage   88.66%   88.60%   -0.06%     
==========================================
  Files         167      167              
  Lines       22873    22905      +32     
  Branches     3478     3483       +5     
==========================================
+ Hits        20280    20296      +16     
- Misses       2038     2054      +16     
  Partials      555      555              
Impacted Files Coverage Δ
tests/filecheck/frontend/dialects/affine.py 52.72% <ø> (ø)
xdsl/dialects/affine.py 76.82% <41.66%> (-15.48%) ⬇️
xdsl/ir/affine/affine_map.py 93.33% <77.77%> (-6.67%) ⬇️
tests/dialects/test_affine.py 100.00% <100.00%> (ø)
xdsl/dialects/builtin.py 84.87% <100.00%> (+0.07%) ⬆️
xdsl/ir/affine/affine_expr.py 91.53% <100.00%> (+0.09%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@compor compor left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 18 to 20
@staticmethod
def from_constant(value: int) -> AffineMap:
return AffineMap(0, 0, [AffineExpr.constant(value)])
Copy link
Collaborator

@Groverkss Groverkss Jun 30, 2023

Choose a reason for hiding this comment

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

Can we call this method constant_map instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

How about just constant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer constant_map. It's also what mlir uses.

Comment on lines +26 to +29
@staticmethod
def empty() -> AffineMap:
return AffineMap(0, 0, [])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make the empty method with dims = 0, syms = 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean explicitly specifying the arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I meant the api. But it's fine, I can extend this later.

Base automatically changed from sasha/affine/for-results to main July 1, 2023 19:05
@superlopuh superlopuh merged commit 312cd94 into main Jul 3, 2023
11 of 12 checks passed
@superlopuh superlopuh deleted the sasha/affine/maps branch July 3, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants