Skip to content

Add while loop section to parallel model#50

Merged
havogt merged 2 commits intoGridTools:masterfrom
jdahm:patch-1
Jan 25, 2022
Merged

Add while loop section to parallel model#50
havogt merged 2 commits intoGridTools:masterfrom
jdahm:patch-1

Conversation

@jdahm
Copy link
Copy Markdown
Contributor

@jdahm jdahm commented Jan 13, 2022

Describes the syntax and limitation of while loops.

Describes the syntax and limitation of while loops.
Copy link
Copy Markdown
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

Thanks! This summarizes what I was missing to review the implementation PR.

I was thinking a bit more about it and came to the conclusion, that the restriction actually doesn't apply, because it is already covered by rule 4. Therefore it would be good to actually add proper checking for restrictions described here (which probably are currently missing) and remove the restriction on the while loop in the implementation.
Please double check if I got it right or if I missed an important that makes it special.

Comment on lines +324 to +344
however due to the blocking model used, there is no way to enforce
synchronizations between the nested statements, or the mask update.
This is a subtle but important point to remember when employing
while loops. The final parallel model behaves as

```python
for k_ in range(k, K):
parfor ij:
mask[i, j] = (a[i, j, k_] < b[i, j, k_])
while any(mask):
parfor ij:
if mask[i, j]:
c[i, j, k_] += 1
a[i, j, k_] += 1
mask[i, j] = (a[i, j, k_] < b[i, j, k_])
```

The conclusion from this is that the user is not allowed to write to
variables used with a horizontal offset either in the mask or elsewhere in
the body of the while. The gtscript frontend implemented in gt4py contains checks
to ensure that a user cannot write code incompatible with this restriction.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
however due to the blocking model used, there is no way to enforce
synchronizations between the nested statements, or the mask update.
This is a subtle but important point to remember when employing
while loops. The final parallel model behaves as
```python
for k_ in range(k, K):
parfor ij:
mask[i, j] = (a[i, j, k_] < b[i, j, k_])
while any(mask):
parfor ij:
if mask[i, j]:
c[i, j, k_] += 1
a[i, j, k_] += 1
mask[i, j] = (a[i, j, k_] < b[i, j, k_])
```
The conclusion from this is that the user is not allowed to write to
variables used with a horizontal offset either in the mask or elsewhere in
the body of the while. The gtscript frontend implemented in gt4py contains checks
to ensure that a user cannot write code incompatible with this restriction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

within a computation, it is illegal to write to an external (non-temporary) field (or aliases pointing to the same memory location) if it is also read with horizontal offset; this rule does not apply to temporaries,

True, but I believe there still could be issues with the update of the mask. Consider this:

a = input_field # a is a temporary field
while a[1, 0, 0] < 0:
    a += 1
    ...

I suppose the rule should be that there are no horizontal offsets (or vertical offsets for that matter) in the while condition. Does that sound right to you?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it makes sense. It would require a temporary with halo of the size of max iterations that the while loop could have (which is unknown), right?

Comment thread GTScript-Parallel-model.md Outdated
@havogt
Copy link
Copy Markdown
Contributor

havogt commented Jan 17, 2022

The important point is, if I am not mistaken, that the restriction doesn't apply to temporaries and therefore doesn't apply to the mask. Does it make sense?

Co-authored-by: Hannes Vogt <hannes@havogt.de>
Copy link
Copy Markdown
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

Looks good.

@havogt havogt merged commit 962a7f5 into GridTools:master Jan 25, 2022
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