bug[next]: Fix shift / remap lowering #1231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tehrengruber
commented
Apr 12, 2023
| @@ -206,17 +206,19 @@ def _visit_shift(self, node: foast.Call, **kwargs) -> itir.Expr: | |||
| ) | |||
| case _: | |||
| raise FieldOperatorLoweringError("Unexpected shift arguments!") | |||
| return shift_offset(self.visit(node.func, **kwargs)) | |||
| return im.lift_(im.lambda__("it")(im.deref_(shift_offset("it"))))( | |||
There was a problem hiding this comment.
For the reviewer this is the core of this PR.
havogt
reviewed
Apr 13, 2023
Comment on lines
549
to
574
| func_str_repr: str # just for error handling | ||
| if isinstance( | ||
| new_func.type, | ||
| (ts.FunctionType, ts_ffront.FieldOperatorType, ts_ffront.ScanOperatorType), | ||
| ): | ||
| # Since we use the `id` attribute in the latter part of the toolchain ensure we | ||
| # have the proper format here. | ||
| if not isinstance( | ||
| new_func, | ||
| (foast.FunctionDefinition, foast.FieldOperator, foast.ScanOperator, foast.Name), | ||
| ): | ||
| raise FieldOperatorTypeDeductionError.from_foast_node( | ||
| node, msg="Functions can only be called directly!" | ||
| ) | ||
| func_str_repr = new_func.id | ||
| elif isinstance(new_func.type, ts.FieldType): | ||
| func_str_repr = str(new_func) | ||
| else: | ||
| raise FieldOperatorTypeDeductionError.from_foast_node( | ||
| node, | ||
| msg=f"Expression of type `{new_func.type}` is not callable, must be a `Function`, `FieldOperator`, `ScanOperator` or `Field`.", | ||
| ) | ||
|
|
||
| # ensure signature is valid | ||
| try: | ||
| type_info.accepts_args( |
There was a problem hiding this comment.
This snippet, including the type_info.accepts_args is confusing. How does that work in case of new_func.type == FieldType? Let's talk about it.
havogt
reviewed
Apr 20, 2023
| @@ -556,7 +579,7 @@ def visit_Call(self, node: foast.Call, **kwargs) -> foast.Call: | |||
| ) | |||
| except GTTypeError as err: | |||
| raise FieldOperatorTypeDeductionError.from_foast_node( | |||
| node, msg=f"Invalid argument types in call to `{node.func.id}`!" | |||
| node, msg=f"Invalid argument types in call to `{func_str_repr}`!" | |||
There was a problem hiding this comment.
Once you have implemented the change that you wanted to make to avoid func_str_repr, it should be good to go.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on something else I recognized a rather sneaky bug in the lowering of shifts from FOAST to ITIR. Contrary to the rest of the lowering we were not wrapping the shifts inside a lifted stencil, which can lead to bugs in user code. This is particularly devastating when executing with the gtfn backend in Release Mode, as you might just run into a segfault or incorrect results. I am actually rather surprised this did not surface earlier somewhere as something like this
lowers to the (incorrect) ITIR:
(Note how the ordering of the shifts is wrong). I had found this bug much earlier while working on (#965), but sadly the change went lost while merging (was very easy to overlook so @havogt and me bost missed it). What I did not recognize back then was that this bug could actually be triggered. I assumed it only occurred for expression like
field(E2V[0])(C2E[0])which were not allowed (for unrelated reasons). For completeness this PR also adds support for such expressions. In principle it is possible to remove this feature from the PR.