[Parallel intervals] Frontend implementation by jdahm · Pull Request #227 · GridTools/gt4py

jdahm · 2020-10-30T04:04:41Z

Description

Frontend implementation for parallel intervals.

Changes:

New recursive algorithm for parsing ComputationBlocks
AxisBound has a new extend parameter that defaults to False. If this is True, the parallel interval extends infinitely far.

Second PR in the parallel intervals series.

Blocked by:

To do:

Add frontend tests

Requirements

Before submitting this PR, please make sure:

The code builds cleanly without new errors or warnings
The code passes all the existing tests
If this PR adds a new feature, new tests have been added to test these
new features
All relevant documentation has been updated or added

Additionally, if this PR contains code authored by new contributors:

All the authors are covered by a valid contributor assignment agreement,
signed by the employer if needed, provided to ETH Zurich
The names of all the new contributors have been added to an updated
version of the AUTHORS.rst file included in the PR

jdahm · 2020-10-30T04:09:10Z

src/gt4py/frontend/gtscript_frontend.py

+            parallel_intervals.append(
+                [
+                    gt_ir.utils.parse_interval_node(
+                        axis, name, loc=gt_ir.Location.from_ast_node(call_node)
+                    )
+                    for axis, name in zip(axes_slices, axis_names)
+                ]
+            )


This is now able to use the same function for parsing parallel intervals as the sequential axis!

egparedes

I just reviewed the changes since last time I checked and they look fine to me. Anyway, I consider @tehrengruber the main reviewer here and I think he should take a look again to this PR (ideally this week) to give a final round of feedback or approve it.

src/gt4py/frontend/gtscript_frontend.py

Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>

…gions-gdp-frontend

tehrengruber

The parsing of the with statement's is to complicated for my taste, but we can tackle that when the new frontend will land at some point. ~~The syntax should be changed slightly so that regions slices are always 2D, not 1D, rest are only minor things.~~

tehrengruber · 2020-12-09T00:32:11Z

tests/test_unittest/test_gtscript_frontend.py

        assert len(annotations) == 6
+
+
+class TestParallelIntervals:


Your GDP mentions specifying multiple regions at once, e.g. parallel(region[I[0], :], region[I[-1], :]). I couldn't find a test for this, neither here nor in one of the other PRs.

tehrengruber · 2021-02-08T17:25:45Z

src/gt4py/frontend/gtscript_frontend.py


    @staticmethod
-    def single_index_slice(node: ast.Expr):
+    def slice_from_value(node: ast.Expr) -> ast.Slice:


If I get this right, this is responsible for transforming something like I[0], e.g. part of region[I[0], :], into I[0]:I[0]+1. A region is a 2-dimensional entity, while region[I[0], :] suggests it to be one dimensional. I believe what you are doing here should actually be reflected directly in the syntax, i.e. region[I[0]:I[0]+1, :].

I see your point, @tehrengruber, but I'm not so sure about your proposal. The region specification here is still 2D, even if one of the axes only contains one element, and although your proposal is slightly more correct from a mathematical point of view, it's also more verbose and therefore harder to read. Personally, I'd accept the current succinct specification for these cases, since changing the implementation here also involves changing the syntax in the GDP, but I might be wrong...

I pondered with the syntax a little bit more and I'm ok with it now. My reasoning here is/was two-fold. First the syntax adds additional complexity to the backend, which I would like to avoid until it is overhauled at some point. I still believe this is a valid point, but a matter of taste. Secondly I understood the syntax (symbolically) means region[...] returns the slices of the domain to run on, in which case it should not change its dimension. However one can also see it as returning the indices of the domain to run on, where the dimension doesn't matter. I'm not in favor of this perspective, but it's valid one and the verbosity is also a good point.

tehrengruber · 2021-02-08T17:27:29Z

src/gt4py/frontend/gtscript_frontend.py

-            elif isinstance(node.value, int):
-                offset = node.value
+            if isinstance(value, int):
+                level = gt_ir.LevelMarker.END if value < 0 else gt_ir.LevelMarker.START


nitpicking

Suggested change

level = gt_ir.LevelMarker.END if value < 0 else gt_ir.LevelMarker.START

level = gt_ir.LevelMarker.START if value >= 0 else gt_ir.LevelMarker.END

tehrengruber · 2021-02-08T17:30:14Z

src/gt4py/frontend/gtscript_frontend.py

+                level = gt_ir.LevelMarker.START if value.index >= 0 else gt_ir.LevelMarker.END
+                offset = value.index + value.offset
+            elif value is None:
+                LARGE_NUM = np.iinfo(np.int32).max


I would actually use math.inf or something symbolic here. It's clearer while debugging and you use the value symbolically anyway. Otherwise the correct data type is np.uintc.

Suggested change

LARGE_NUM = np.iinfo(np.int32).max

LARGE_NUM = np.iinfo(np.uintc).max

tehrengruber · 2021-02-08T17:38:15Z

tests/test_unittest/test_gtscript_frontend.py

+                field = func(field)
+
+        module = f"TestParallelIntervals_func_and_externals_{id_version}"
+        externals = {"ext": I[0] - np.iinfo(np.int32).max}


Suggested change

externals = {"ext": I[0] - np.iinfo(np.int32).max}

externals = {"ext": I[0] - np.iinfo(np.uintc).max}

tehrengruber · 2021-02-08T17:47:47Z

src/gt4py/frontend/gtscript_frontend.py

+                level=left.level, offset=bin_op(left.offset, right), loc=self.loc
+            )
+        elif isinstance(left, numbers.Number) and isinstance(right, numbers.Number):
+            return bin_op(left, right)


If no condition applies an exception should be raised.

tehrengruber · 2021-02-08T18:56:01Z

src/gt4py/frontend/gtscript_frontend.py

        ast.copy_location(assignment, node)
        return self.visit_Assign(assignment)

+    def _unroll_computations(self, node: ast.With):


Suggested change

def _unroll_computations(self, node: ast.With):

def _flatten_computations(self, node: ast.With):

I see the similarity with loop unrolling, but I would call this flattening.

tehrengruber · 2021-02-08T18:57:10Z

src/gt4py/frontend/gtscript_frontend.py

+
+        parallel_intervals = []
+        for node in call_node.args:
+            if isinstance(node.slice, ast.ExtSlice):


It would greatly increase readability if you could add a comment for each of the cases with a syntax example.

tehrengruber · 2021-02-08T19:04:18Z

src/gt4py/frontend/gtscript_frontend.py

-    COMPUTATION = 2
-
-
 class IRMaker(ast.NodeVisitor):


With the addition of the parallel intervals, it's very hard to understand how with statements are parsed. Creating an additional cannonicalization from python ast -> python ast that transforms into a flat representation would make sense. However since the frontend is to be refactored anyway at some point I'm fine with this right now.

tehrengruber · 2021-02-08T19:07:07Z

src/gt4py/frontend/gtscript_frontend.py

        # Parse computation specification, i.e. `withItems` nodes
        iteration_order = None
        interval = None
+        parallel_intervals = [None]


For computations without a parallel interval specification there should still be parallel intervals, but from -inf to inf right?

jdahm added 18 commits October 28, 2020 16:06

Add intervals tests and rm stray adds to AxisBound

81e3bbd

Switch to new parser

f59b8be

Enable new syntax

b7f32ac

Finish making tests pass

13970f4

Reverse interval test

5acb589

Disable VarRef in gt_backends to avoid mypy error

066a43d

Fix for Python 3.8

865d5b2

Make name easier to understand

d0c3a19

Correctly check None

99e2b47

Remove former make_axis_interval

ef933d0

Move AxisBoundParser to gt4py.ir.utils temporarily

45bb93d

Add to ComputationBlock node

b190cf0

Skeleton implementation

07a6766

Switch to building entire interval

f45af01

eval_external -> eval_constant

87f3692

Fix return type

24b260e

Merge branch 'interval-syntax' into regions-gdp-frontend

dd7aefa

Parse regions correctly in frontend

995eb1d

jdahm commented Oct 30, 2020

View reviewed changes

jdahm added 11 commits October 29, 2020 21:33

Add tests and make small fix for single points

e970d8a

Allow for removing regions

c4d5bd2

Test enforcing error if None at start of interval

c0a4dac

Merge branch 'interval-syntax' into regions-gdp-frontend

95ce518

Merge branch 'master' into interval-syntax

67572de

Merge branch 'interval-syntax' into regions-gdp-frontend

a9a272b

Remove unused code

cb82cbc

Merge branch 'remove-dead-code-tests' into interval-syntax

9aa8c58

Move AxisIntervalParser to gtscript_frontend.py

0e6c882

Fix undefined references

8a3e915

Fix undefined reference

b66e796

jdahm added 19 commits December 1, 2020 12:21

Add another test for corner case

c69e226

Merge branch 'support-imports-functions' into regions-gdp-frontend

c5af015

Expand AxisOffset capability: store index, offset

f04a300

Fix for Python 3.8+

ea24053

Correct check

1745bb1

Add subtraction to AxisOffset

79fc36b

Add __eq__ to AxisOffset

b83c0cb

Remove None, use infinite extensions

ce6e663

Merge branch 'master' into regions-gdp-frontend

c2ecdb6

Remove RegionRemover

ee24e4a

Use horizontal instead of parallel

109ece3

gtscript function for horizontal

564c88a

Fix tests

5d7b95e

Merge branch 'master' into regions-gdp-frontend

d24c79f

Remove extension to infinity

8c60923

Only recurse unroll if there are stmts available

f38a2ef

Add test with both I and J restriction

d9b2e5a

Merge branch 'master' into regions-gdp-frontend

264c746

Skip remapping externals in CallInliner

3a4d7bc

egparedes reviewed Feb 4, 2021

View reviewed changes

src/gt4py/frontend/gtscript_frontend.py Outdated Show resolved Hide resolved

jdahm and others added 7 commits February 4, 2021 10:51

Update src/gt4py/frontend/gtscript_frontend.py

7e3a779

Co-authored-by: Enrique G. Paredes <18477+egparedes@users.noreply.github.com>

Fix test

0a6f02c

Merge branch 'regions-gdp-frontend' of github.com:jdahm/gt4py into re…

0f9fc09

…gions-gdp-frontend

Merge branch 'master' into regions-gdp-frontend

3292632

Rewrite AxisIntervalParser for Python 3.8+

a60c704

offset fix

892716e

Revert unnnecessary additions

ed2a0c7

tehrengruber requested changes Feb 8, 2021

View reviewed changes

Merge branch 'master' into regions-gdp-frontend

88b1874

jdahm closed this Feb 17, 2021

	level = gt_ir.LevelMarker.END if value < 0 else gt_ir.LevelMarker.START
	level = gt_ir.LevelMarker.START if value >= 0 else gt_ir.LevelMarker.END

	LARGE_NUM = np.iinfo(np.int32).max
	LARGE_NUM = np.iinfo(np.uintc).max

	externals = {"ext": I[0] - np.iinfo(np.int32).max}
	externals = {"ext": I[0] - np.iinfo(np.uintc).max}

	def _unroll_computations(self, node: ast.With):
	def _flatten_computations(self, node: ast.With):

Conversation

jdahm commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Requirements

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egparedes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tehrengruber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egparedes Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jdahm commented Oct 30, 2020 •

edited

Loading

tehrengruber left a comment •

edited

Loading

egparedes Feb 9, 2021 •

edited

Loading