-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat[eve]: Preserve annex in custom visitors #1874
base: main
Are you sure you want to change the base?
Changes from 5 commits
06806fb
bab4fe1
c5fba83
04ae430
5136adc
9653694
dfe819c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,7 @@ | |
domain_utils, | ||
ir_makers as im, | ||
) | ||
from gt4py.next.iterator.transforms import trace_shifts | ||
from gt4py.next.iterator.transforms import constant_folding, trace_shifts | ||
from gt4py.next.utils import flatten_nested_tuple, tree_map | ||
|
||
|
||
|
@@ -436,8 +436,12 @@ def _infer_stmt( | |
**kwargs: Unpack[InferenceOptions], | ||
): | ||
if isinstance(stmt, itir.SetAt): | ||
# constant fold once otherwise constant folding after domain inference might create (syntactic) differences | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this change really belong to this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I initially checked that the new annex attribute is equal to the old one. That would be a desirable property of the IR and this change here is a step in that direction. This also motivated the comment above
However there are annex values that are equivalent, but not equal e.g. the |
||
# between the domain stored in IR and in the annex | ||
domain = constant_folding.ConstantFolding.apply(stmt.domain) | ||
|
||
transformed_call, _ = infer_expr( | ||
stmt.expr, domain_utils.SymbolicDomain.from_expr(stmt.domain), **kwargs | ||
stmt.expr, domain_utils.SymbolicDomain.from_expr(domain), **kwargs | ||
) | ||
|
||
return itir.SetAt( | ||
|
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.
What do you think about leaving the condition outside of the function? This is probably the hottest loop in the toolchain so I'd rather try to avoid a unnecessary function call if possible. Additionally, it would also make the function name fully correct 😄
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 thought about this, but went for not duplicating the condition. Makes sense to me though, so I reverted that.