From 829b2eb8744261023bfc8c15f728044cbe03d9db Mon Sep 17 00:00:00 2001 From: Roman Cattaneo Date: Wed, 4 Sep 2024 18:07:18 +0200 Subject: [PATCH 1/2] docs: Fixed typos in ADRs and a docstring (#1627) I noticed a couple of typos in the ADRs (and in one docstring) while I was familiarizing myself with the codebase. Thanks for keeping an ADR and good documentation practices. Really helps if you are new to a codebase. This PR just fixes mentioned typos. Co-authored-by: Roman Cattaneo <> --- docs/development/ADRs/0001-Field_View_Frontend_Design.md | 2 +- docs/development/ADRs/0002-Field_View_Lowering.md | 2 +- docs/development/ADRs/0009-Compiled-Backend-Integration.md | 2 +- docs/development/ADRs/0010-Domain_in_Field_View.md | 2 +- docs/development/ADRs/0011-On_The_Fly_Compilation.md | 4 ++-- docs/development/ADRs/0014-DaCe_backend.md | 2 +- docs/development/ADRs/0017-Toolchain-Configuration.md | 2 +- .../cartesian/gtc/passes/oir_optimizations/temporaries.py | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/development/ADRs/0001-Field_View_Frontend_Design.md b/docs/development/ADRs/0001-Field_View_Frontend_Design.md index cb04bec27c..4e21329096 100644 --- a/docs/development/ADRs/0001-Field_View_Frontend_Design.md +++ b/docs/development/ADRs/0001-Field_View_Frontend_Design.md @@ -120,7 +120,7 @@ Parses from Python AST into PAST. Lowers from PAST to Iterator IR. **Field operator out argument slicing** -The lowering contains some complex logic to emulate slicing of fields in order to restrict the output domain of a field operator call. This contradics guiding principle (3) and should be removed in the future. The concept of specifying the domain for Field operators was not investigated during the frontend design as we concentrated on the beautified iterator dialect where the output domain is explicitly given when lifting a local operator to a field operator. The alternative to also explicitly specify the domain for calls to field operators was rejected as it: +The lowering contains some complex logic to emulate slicing of fields in order to restrict the output domain of a field operator call. This contradicts guiding principle (3) and should be removed in the future. The concept of specifying the domain for Field operators was not investigated during the frontend design as we concentrated on the beautified iterator dialect where the output domain is explicitly given when lifting a local operator to a field operator. The alternative to also explicitly specify the domain for calls to field operators was rejected as it: - would require introducing significantly more syntax in order to specify domains, which was not only infeasible to implement in time, but there also didn't exist an accepted syntax to do so. - no intuitive syntax was found to do this diff --git a/docs/development/ADRs/0002-Field_View_Lowering.md b/docs/development/ADRs/0002-Field_View_Lowering.md index a1611c1d06..c6c358bd0c 100644 --- a/docs/development/ADRs/0002-Field_View_Lowering.md +++ b/docs/development/ADRs/0002-Field_View_Lowering.md @@ -179,4 +179,4 @@ Since temporary variables are no longer inlined, the renaming that happens in th ## Operator signature FOAST <-> ITIR -On iterator level the arguments to all stencils / functions used inside a fencil closure need to be iterators (due to the compiled backend using a single SID composite to pass the arguments). Following the FOAST value <-> ITIR value, FOAST field <-> ITIR iterator correspondence, all field operator arguments whose type on FOAST level is a value, i.e. scalar or composite thereof, are expected to be values on ITIR level by the rest of the lowering. As a consequence we transform all values into iterators before calling field operators (to satisfy the former constraint) and deref them immediately inside every field operator (to satify the latter constraint). +On iterator level the arguments to all stencils / functions used inside a fencil closure need to be iterators (due to the compiled backend using a single SID composite to pass the arguments). Following the FOAST value <-> ITIR value, FOAST field <-> ITIR iterator correspondence, all field operator arguments whose type on FOAST level is a value, i.e. scalar or composite thereof, are expected to be values on ITIR level by the rest of the lowering. As a consequence we transform all values into iterators before calling field operators (to satisfy the former constraint) and deref them immediately inside every field operator (to satisfy the latter constraint). diff --git a/docs/development/ADRs/0009-Compiled-Backend-Integration.md b/docs/development/ADRs/0009-Compiled-Backend-Integration.md index 27c2f0c73c..ed2f65fb2f 100644 --- a/docs/development/ADRs/0009-Compiled-Backend-Integration.md +++ b/docs/development/ADRs/0009-Compiled-Backend-Integration.md @@ -56,7 +56,7 @@ The interface for step two is defined in the `fencil_processors.pipeline.Binding The output of this step is a `BindingsModule` instance and also safely hashable. -This would could be usefeul as an endpoint to use the Python bindings as an example for handcrafted bindings to other languages or for distribution of the generated bindings in a self-contained library with it's own build system. +This would could be useful as an endpoint to use the Python bindings as an example for handcrafted bindings to other languages or for distribution of the generated bindings in a self-contained library with it's own build system. **Step 3a:** diff --git a/docs/development/ADRs/0010-Domain_in_Field_View.md b/docs/development/ADRs/0010-Domain_in_Field_View.md index cb4ec3134a..54d587939c 100644 --- a/docs/development/ADRs/0010-Domain_in_Field_View.md +++ b/docs/development/ADRs/0010-Domain_in_Field_View.md @@ -14,7 +14,7 @@ the domain was deduced relative to _the_ `out` argument. We decided to deduce th ## Context -The compute domain in field view is deduced relative to the shape of fields in the `out` argument. This is straigh-forward for a single field. +The compute domain in field view is deduced relative to the shape of fields in the `out` argument. This is straight-forward for a single field. For multiple fields we would need to check at call-time if all fields are sliced in the correct way. Additionally, expressing absolute domains is desirable for some use-cases. In summary, we need to revisit the domain handling in field view. diff --git a/docs/development/ADRs/0011-On_The_Fly_Compilation.md b/docs/development/ADRs/0011-On_The_Fly_Compilation.md index 14f61bf0d4..697fdf7253 100644 --- a/docs/development/ADRs/0011-On_The_Fly_Compilation.md +++ b/docs/development/ADRs/0011-On_The_Fly_Compilation.md @@ -195,7 +195,7 @@ _edit 2023-03-29_ `NamedStepSequence` solves the following problem: -By making it possible to quickly create a variant of an existing workflow (using `workflow.replace`), they make it unneccessary to escalate sub-workflow configuration. +By making it possible to quickly create a variant of an existing workflow (using `workflow.replace`), they make it unnecessary to escalate sub-workflow configuration. Coming back to the above example of `ExampleWorkflow`, instead of: @@ -277,7 +277,7 @@ This greatly simplifies accessing already compiled and cached (across runs) GT4P Whereas when using the `BuildData` one only requires to run steps 1) and 2) to get the cached project folder and the rest can be accessed from there. These steps can be skipped as well by implementing more aggressive caching (for production environments where the GT4Py source is kept constant). -Looking at the build systems implemented at the time of writing, `CMakeProject` and `CompiledbProject`, one might wrongly conclude that existence of certain directories or files suffice. This is only the case because `CompiledbProject` is based on `CMakeProject` and they therefore share some implementation details, which are by no means guaranteed to be standardizeable across multiple conceptually different build systems. +Looking at the build systems implemented at the time of writing, `CMakeProject` and `CompiledbProject`, one might wrongly conclude that existence of certain directories or files suffice. This is only the case because `CompiledbProject` is based on `CMakeProject` and they therefore share some implementation details, which are by no means guaranteed to be standardizable across multiple conceptually different build systems. #### Revise If diff --git a/docs/development/ADRs/0014-DaCe_backend.md b/docs/development/ADRs/0014-DaCe_backend.md index a4eb9c7e9b..8ad7aaee31 100644 --- a/docs/development/ADRs/0014-DaCe_backend.md +++ b/docs/development/ADRs/0014-DaCe_backend.md @@ -78,7 +78,7 @@ After a cycle, we have a working implementation of the iterator IR to DaCe SDFG **Solution**: The iterator type inference pass should provide the concrete type of all value-expressions. -#### Partial suppot for iterator IR grammar +#### Partial support for iterator IR grammar - First order functions: in DaCe SDFG's, first order functions could be represented by associating a nested SDFG (function equivalent in DaCe) with an access node (variable equivalent in DaCe), which is not a thing as far as I know. There is no practical solution to support this in the SDFG lowering, such ITIR will always be rejected and should not be generated from ITIR passes. - Lambdas: in DaCe SDFG's, an immediate call to an instantiation of a lambda function can be represented as a nested SDFG. Currently, the DaCe backend does not support this, but the solution should be fairly straightforward. Lambda support is necessary as they are essential for CSE in ITIR. diff --git a/docs/development/ADRs/0017-Toolchain-Configuration.md b/docs/development/ADRs/0017-Toolchain-Configuration.md index 3448314316..4992ef1df8 100644 --- a/docs/development/ADRs/0017-Toolchain-Configuration.md +++ b/docs/development/ADRs/0017-Toolchain-Configuration.md @@ -26,7 +26,7 @@ At the time of creation of this document, at least one additional toolchain is a - Some ways of configuring toolchains involve configuring multiple components in synch - Some ways of configuring toolchains involve switching out or nesting toolchain steps -- What configuration options are avaiable will depend on what toolchain will be used and how that is configured. +- What configuration options are available will depend on what toolchain will be used and how that is configured. - Hierarchical configuration defaults and overrides can be confusing from a user perspective. - Leaving the configuration interface completely up to toolchain developers could lead to a confusing ad fragmented user experience. diff --git a/src/gt4py/cartesian/gtc/passes/oir_optimizations/temporaries.py b/src/gt4py/cartesian/gtc/passes/oir_optimizations/temporaries.py index 1a9e13534c..2498dd0278 100644 --- a/src/gt4py/cartesian/gtc/passes/oir_optimizations/temporaries.py +++ b/src/gt4py/cartesian/gtc/passes/oir_optimizations/temporaries.py @@ -117,7 +117,7 @@ def visit_Stencil(self, node: oir.Stencil, **kwargs: Any) -> oir.Stencil: class WriteBeforeReadTemporariesToScalars(TemporariesToScalarsBase): - """Replaces temporay fields that are always written before read by scalars. + """Replaces temporary fields that are always written before read by scalars. Note that temporaries used in horizontal regions in a single horizontal execution may not be scalarized. From 07f37fbb41ddfae81f69ead674565d76120d0911 Mon Sep 17 00:00:00 2001 From: Roman Cattaneo Date: Wed, 4 Sep 2024 18:09:39 +0200 Subject: [PATCH 2/2] refactor[cartesian]: Enable MaskInlining in gt4yp - DaCe bridge (#1624) `MaskInlining` will be a key feature of the improved gt4py / DaCe bridge as envisioned in https://github.com/GEOS-ESM/NDSL/issues/53, which will propagate more control flow elements from gt4py to DaCe. We thus want to make sure if `MaskInlining` itself is posing any problems. We tracked the git history of this explicit exclude back to PR https://github.com/GridTools/gt4py/pull/300, which not only added the npir backend, but also refactored how pipeline stages are defined. Previous to this PR optimization passes to be specified explicitly and the PR changed this to an exclude list. It looks like the `MaskInlining` pass was added after the list of optimization passes was formed and thus excluded (to keep the list constant). From this history we thus don't expect `MaskInlining` to break anything. Co-authored-by: Roman Cattaneo <> --- src/gt4py/cartesian/backend/dace_backend.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/gt4py/cartesian/backend/dace_backend.py b/src/gt4py/cartesian/backend/dace_backend.py index 7608fcd5e7..b9cfa8c727 100644 --- a/src/gt4py/cartesian/backend/dace_backend.py +++ b/src/gt4py/cartesian/backend/dace_backend.py @@ -42,7 +42,6 @@ from gt4py.cartesian.gtc.gtir_to_oir import GTIRToOIR from gt4py.cartesian.gtc.passes.gtir_k_boundary import compute_k_boundary from gt4py.cartesian.gtc.passes.gtir_pipeline import GtirPipeline -from gt4py.cartesian.gtc.passes.oir_optimizations.inlining import MaskInlining from gt4py.cartesian.gtc.passes.oir_optimizations.utils import compute_fields_extents from gt4py.cartesian.gtc.passes.oir_pipeline import DefaultPipeline from gt4py.cartesian.utils import shash @@ -353,7 +352,7 @@ def _unexpanded_sdfg(self): except FileNotFoundError: base_oir = GTIRToOIR().visit(self.builder.gtir) oir_pipeline = self.builder.options.backend_opts.get( - "oir_pipeline", DefaultPipeline(skip=[MaskInlining]) + "oir_pipeline", DefaultPipeline() ) oir_node = oir_pipeline.run(base_oir) sdfg = OirSDFGBuilder().visit(oir_node)