-
Notifications
You must be signed in to change notification settings - Fork 73
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
add TaggedExpression #688
base: main
Are you sure you want to change the base?
add TaggedExpression #688
Conversation
7b09146
to
fe827e4
Compare
Is this going in the direction you had in mind @inducer ? |
ae1f27a
to
e41625f
Compare
e41625f
to
9233385
Compare
loopy/statistics.py
Outdated
opmap = self.rec(expr.expr) | ||
for op in opmap.count_map: | ||
op.tags = expr.tags | ||
return opmap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would overwrite tags of subexpressions that already have tags.
self.rec(expr.expr, expr.tags)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 50adbc4 what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, generally.
loopy/statistics.py
Outdated
|
||
map_bitwise_xor = map_bitwise_or | ||
map_bitwise_and = map_bitwise_or | ||
|
||
def map_if(self, expr): | ||
def map_if(self, expr, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why switch to *args
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 0bbbaec, I changed all occurrences to tags
(with a default argument). Does this look reasonable?
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
This is ready for a first look @inducer. |
loopy/statistics.py
Outdated
@@ -916,14 +931,17 @@ def __init__(self, knl, callables_table, kernel_rec, | |||
def combine(self, values): | |||
return sum(values) | |||
|
|||
def map_constant(self, expr): | |||
def map_constant(self, expr, tags=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the pervasive tags=None
? The default risks losing the tag as you traverse. IMO, there should not be a default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it in df7ed9b to not use a default argument anymore.
loopy/statistics.py
Outdated
else: | ||
return self.new_zero_poly_map() | ||
|
||
map_product = map_sum | ||
|
||
def map_comparison(self, expr): | ||
return self.rec(expr.left)+self.rec(expr.right) | ||
def map_comparison(self, expr, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have *args
instead of tags
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are hopefully all fixed in df7ed9b
48b6b94
to
3f9df15
Compare
4d9e458
to
a693fb0
Compare
This is ready for another look @inducer |
This reverts commit b54217d.
This comment was marked as outdated.
This comment was marked as outdated.
aa4f1d7
to
dd419b0
Compare
TODOs:
Please squash