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

bug[next]: itir.embedded: fix shift inside scan pass #1280

Merged
merged 7 commits into from
Jul 4, 2023

Conversation

tehrengruber
Copy link
Contributor

@tehrengruber tehrengruber commented Jul 3, 2023

Shifts were ignored in the ScanArgIterator, i.e. all shifts in the scan_pass were ignored. This partially fixes the test_icon_like_scan.

Additional:

  • Run InlineIntoScan after temporary creation to inline remaining operations that block bubbling scans to the top.

@@ -1196,7 +1195,7 @@ def can_deref(self) -> bool:
return self.wrapped_iter.can_deref()

def shift(self, *offsets: OffsetPart) -> ScanArgIterator:
return ScanArgIterator(self.wrapped_iter, self.k_pos, offsets=[*offsets, *self.offsets])
return ScanArgIterator(self.wrapped_iter.shift(*offsets), self.k_pos)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the important part.

Comment on lines 1394 to 1399
if isinstance(res, tuple):
out.field_setitem(
ordered_indices, tuple(res[i][k] for i in range(len(res)))
) # TODO(tehrengruber): only works for scalars
else:
out.field_setitem(ordered_indices, res[k])
Copy link
Contributor

Choose a reason for hiding this comment

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

is this fixed in https://github.com/GridTools/gt4py/pull/1141/files? or is it another instance of that problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure actually, might be two different things. This is not clean however as it only works for tuples not nested tuples.

@@ -46,7 +46,7 @@ def shifted(inp):

@fundef
def wrapped(inp):
return scan(sum, True, 0.0)(inp, lift(shifted)(inp))
return scan(sum, True, 0.0)(inp, shift(Koff, 1)(inp))
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks something else, if we undo we could merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I addressed this issue by adding another inline_into_scan pass after temporaries are created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We concluded that this is fine for now even though we are not sure whether the ordering of the passes makes sense anymore. This will be addressed as a whole in another PR.

@tehrengruber
Copy link
Contributor Author

The test failures seem to be a result of removing https://github.com/GridTools/gt4py/pull/1280/files/d4160bb32c7e091e94afe983fbe747c32c9f7514#r1250968455 again.

@havogt havogt changed the title bug[next]: Fix shift inside scan pass bug[next]: itir.embedded: fix shift inside scan pass Jul 4, 2023
@tehrengruber tehrengruber merged commit a61efd0 into GridTools:main Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants