Generalize Targets as Operators #1028

FabioLuporini · 2019-12-17T10:15:33Z

This PR builds on top of #1026 and #1027 . It basically generalises the Target concept introduced in these two preliminary PRs.

Again, no new functionality is added. This PR is a mere refactoring aimed at simplifying one's life when it gets to specialising compilation passes for a given <platform, mode, language>

CavalcanteLucas · 2019-12-19T17:15:21Z

devito/operator/__init__.py

 from .profiling import profiler_registry  # noqa
+from .registry import operator_registry  # noqa
+from .operator import Operator  # noqa


not sure if intended, but these seem to be in a (almost)reversed alphabetical order

ahhhh. OK, fixing

devito/operator/operator.py

CavalcanteLucas · 2019-12-19T17:55:06Z

devito/operator/registry.py

+
+        * `platform` is an object of type Platform, that is the architecture
+          the generated code should be specializer for.
+        * `mode` is the optimization level (e.g., `advanced`)


yeah. Fixing

CavalcanteLucas · 2019-12-19T17:57:59Z

devito/operator/operator.py

@@ -906,4 +911,8 @@ def parse_kwargs(**kwargs):
        except:
            raise InvalidOperator("Illegal `dse=%s`" % str(dse))

+    # Attach `platform` too for convenience, basically so that we don't always have
+    # to query `configuration


missing a closing `

maybe rewrite it. it's a bit confusing..
what about:

Attach `platform` too for convenience, so we don't have to always query for `configuration`

codecov · 2020-01-08T09:37:15Z

Codecov Report

Merging #1028 into master will decrease coverage by 0.04%.
The diff coverage is 92.09%.

@@            Coverage Diff             @@
##           master    #1028      +/-   ##
==========================================
- Coverage   90.66%   90.61%   -0.05%     
==========================================
  Files         166      164       -2     
  Lines       23127    23161      +34     
  Branches     3325     3326       +1     
==========================================
+ Hits        20968    20988      +20     
- Misses       1753     1765      +12     
- Partials      406      408       +2

Impacted Files	Coverage Δ
devito/ir/support/properties.py	`100% <ø> (ø)`	⬆️
devito/core/operator.py	`94.11% <100%> (ø)`	⬆️
devito/passes/iet/mpi.py	`97.75% <100%> (ø)`
devito/core/__init__.py	`100% <100%> (ø)`	⬆️
devito/passes/iet/engine.py	`96% <100%> (ø)`
tests/test_autotuner.py	`100% <100%> (ø)`	⬆️
devito/ops/__init__.py	`100% <100%> (ø)`	⬆️
devito/passes/__init__.py	`100% <100%> (ø)`
devito/passes/iet/__init__.py	`100% <100%> (ø)`
tests/test_operator.py	`97.91% <100%> (ø)`	⬆️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46cf0ef...e33e6a3. Read the comment docs.

georgebisbas

Only did a first pass.

georgebisbas · 2020-01-15T13:31:21Z

devito/operator/operator.py

-        # Lower Clusters to a Schedule tree
-        stree = st_build(clusters)
+        # Turn Clusters into a Schedule tree
+        stree = cls._lower_stree(clusters, **kwargs)


I am a bit confused here. Lower Clusters is changed to Turn into...
However the new name of the function is lower_stree ?

yeah it's not very clear is it? I will revert to "lower". Good catch

georgebisbas · 2020-01-15T13:32:37Z

devito/operator/operator.py

-        # Lower IET to a Target-specific IET
-        op, target_state = cls._specialize_iet(op, **kwargs)
+        # Lower Schedule tree to an Iteration/Expression tree
+        iet, byproduct = cls._lower_iet(stree, profiler, **kwargs)


Isn;it better to have something like lower_to_iet
Seems like the difference between lower sth to lower_to_sth ?

maybe yes?

it's deliberately ambiguous I'd say. While it's true that a ScheduleTree is lowered TO an IET, it is also true that the lowered IET goes through several other sub-passes (which are increasingly lowering the IET itself). So in a certain sense we're also lowering the IET too

Makes sense?

What you say makes sense.
However:
WHat I see is a y=f(x) where x=stree and y=iet and f=lower_iet
So it may make absolute sense, however I would like to eliminate a bit of the ambiguity.
I think adding a line or two of docstrings with some more info along what you wrote would be better.

georgebisbas · 2020-01-15T13:34:23Z

devito/operator/registry.py

+    where:
+
+        * `platform` is an object of type Platform, that is the architecture
+          the generated code should be specializer for.


specialized?

yes. Fixing.

mloubout · 2020-01-15T13:44:39Z

devito/core/cpu.py

+
+        # Force-call `openmp` if requested via global option
+        if 'openmp' not in passes and options['openmp']:
+            passes_mapper['openmp'](graph)


For mpi sure, but this one is by default to using it so maybe shouldn't if not asked for in the custom options?

I think I'm just replicating master's behavior ?

we only end up here when dle != noop/advanced , eg

Operator(..., dle=('blocking', 'simd', X))

all the code is doing is that if 'openmp' doesn't appear among X above, then it depends on the value of DEVITO_OPENMP (which is encoded in options['openmp'])

mloubout · 2020-01-15T13:46:26Z

devito/operator/operator.py


        # Make it an actual Operator
-        op = Callable.__new__(cls, **op.args)
+        op = Callable.__new__(cls, **iet.args)


Still not sure why init not called in new.

cls is an Operator, so it will indeed call Operator.__init__ that is a no-op however. Then, I have to explicitly call Callable's __init__ (line below)

mloubout · 2020-01-15T13:47:52Z

devito/operator/operator.py

+
+            * Group expressions into Clusters;
+            * Optimize Clusters for performance (flops, data locality, ...);
+            * Introduce guards for conditional Clusters


Shouldn't this be just "Specialize for ..." then these two in _specialize_clusters?

good catch. Only the latter though. Will adjust

ah, no: it is gonna be like that I think as of #1030 , but not in this PR yet. With this PR, both optimization and guarding occur through clusterize, hence the docstring is correct. Will have to update it in #1030 though.

mloubout · 2020-01-15T13:48:20Z

devito/operator/operator.py

+
+            * Turn a sequence of Clusters into a ScheduleTree;
+            * Derive and attach metadata for distributed-memory parallelism;
+            * Derive sections for performance profiling


Both Drive on specialize?

No, they are compulsory in lower.

_specialize_stree is currently a no-op

mloubout · 2020-01-15T13:48:37Z

devito/operator/operator.py

+            * Perform analysis to detect optimization opportunities;
+            * Introduce distributed-memory, shared-memory, and SIMD parallelism;
+            * Introduce optimizations for data locality;
+            * Finalization (e.g., symbol definitions, array casts)


FabioLuporini

Addressed first round of comments

FabioLuporini · 2020-01-16T10:51:56Z

devito/core/cpu.py

+
+        # Force-call `openmp` if requested via global option
+        if 'openmp' not in passes and options['openmp']:
+            passes_mapper['openmp'](graph)


I think I'm just replicating master's behavior ?

we only end up here when dle != noop/advanced , eg

Operator(..., dle=('blocking', 'simd', X))

all the code is doing is that if 'openmp' doesn't appear among X above, then it depends on the value of DEVITO_OPENMP (which is encoded in options['openmp'])

FabioLuporini · 2020-01-16T10:52:37Z

devito/operator/operator.py

-        # Lower Clusters to a Schedule tree
-        stree = st_build(clusters)
+        # Turn Clusters into a Schedule tree
+        stree = cls._lower_stree(clusters, **kwargs)


yeah it's not very clear is it? I will revert to "lower". Good catch

FabioLuporini · 2020-01-16T10:56:00Z

devito/operator/operator.py

-        # Lower IET to a Target-specific IET
-        op, target_state = cls._specialize_iet(op, **kwargs)
+        # Lower Schedule tree to an Iteration/Expression tree
+        iet, byproduct = cls._lower_iet(stree, profiler, **kwargs)


maybe yes?

it's deliberately ambiguous I'd say. While it's true that a ScheduleTree is lowered TO an IET, it is also true that the lowered IET goes through several other sub-passes (which are increasingly lowering the IET itself). So in a certain sense we're also lowering the IET too

Makes sense?

FabioLuporini · 2020-01-16T10:58:32Z

devito/operator/operator.py


        # Make it an actual Operator
-        op = Callable.__new__(cls, **op.args)
+        op = Callable.__new__(cls, **iet.args)


cls is an Operator, so it will indeed call Operator.__init__ that is a no-op however. Then, I have to explicitly call Callable's __init__ (line below)

FabioLuporini · 2020-01-16T10:59:16Z

devito/operator/operator.py

+
+            * Group expressions into Clusters;
+            * Optimize Clusters for performance (flops, data locality, ...);
+            * Introduce guards for conditional Clusters


good catch. Only the latter though. Will adjust

FabioLuporini · 2020-01-16T11:00:30Z

devito/operator/operator.py

+
+            * Group expressions into Clusters;
+            * Optimize Clusters for performance (flops, data locality, ...);
+            * Introduce guards for conditional Clusters


ah, no: it is gonna be like that I think as of #1030 , but not in this PR yet. With this PR, both optimization and guarding occur through clusterize, hence the docstring is correct. Will have to update it in #1030 though.

FabioLuporini · 2020-01-16T11:01:22Z

devito/operator/operator.py

+
+            * Turn a sequence of Clusters into a ScheduleTree;
+            * Derive and attach metadata for distributed-memory parallelism;
+            * Derive sections for performance profiling


No, they are compulsory in lower.

_specialize_stree is currently a no-op

FabioLuporini · 2020-01-16T11:02:02Z

devito/operator/operator.py

+            * Perform analysis to detect optimization opportunities;
+            * Introduce distributed-memory, shared-memory, and SIMD parallelism;
+            * Introduce optimizations for data locality;
+            * Finalization (e.g., symbol definitions, array casts)


FabioLuporini · 2020-01-16T11:02:32Z

devito/operator/registry.py

+    where:
+
+        * `platform` is an object of type Platform, that is the architecture
+          the generated code should be specializer for.


yes. Fixing.

georgebisbas

Maybe some of my comments seem nitpicking but all I is do driven from what I feel I need to understand ;-)

georgebisbas · 2020-01-16T11:25:40Z

devito/operator/operator.py

-        # Lower IET to a Target-specific IET
-        op, target_state = cls._specialize_iet(op, **kwargs)
+        # Lower Schedule tree to an Iteration/Expression tree
+        iet, byproduct = cls._lower_iet(stree, profiler, **kwargs)


What you say makes sense.
However:
WHat I see is a y=f(x) where x=stree and y=iet and f=lower_iet
So it may make absolute sense, however I would like to eliminate a bit of the ambiguity.
I think adding a line or two of docstrings with some more info along what you wrote would be better.

georgebisbas · 2020-01-16T11:28:10Z

devito/operator/operator.py

        Callable.__init__(op, **op.args)

        # Header files, etc.
        op._headers = list(cls._default_headers)
-        op._headers.extend(target_state.headers)
+        op._headers.extend(byproduct.headers)


My VScode pylint says
Instance of 'Callable' has no '_headers' member on this line?
SHould we care?

Maybe some info about byproduct? Like a docstring?

Maybe what I say its driven form the fact that byproduct is a bit vague.

My VScode pylint

it's buggy. op is of type Operator

Maybe some info about byproduct? Like a docstring?

I could..... but it feels like overkill. byproduct is simply the by-product of lower-iet which is a bag of additional objects and properties

georgebisbas · 2020-01-16T11:31:56Z

devito/operator/registry.py

+        * `platform` is an object of type Platform, that is the architecture
+          the generated code should be specialized for.
+        * `mode` is the optimization level (e.g., `advanced`).
+        * `language` is the generated code language (default is C+OpenMP+MPI,


Missing parenthesis closing

FabioLuporini · 2020-01-16T17:25:34Z

approved -> merged

CavalcanteLucas reviewed Dec 19, 2019

View reviewed changes

FabioLuporini mentioned this pull request Dec 20, 2019

Generalize DSE as compiler passes #1030

Merged

FabioLuporini force-pushed the refactor-as-operators branch 2 times, most recently from 1d2b9cd to fec5d95 Compare January 8, 2020 09:37

georgebisbas reviewed Jan 8, 2020

View reviewed changes

FabioLuporini added 15 commits January 15, 2020 13:59

targets: Refactor Target __init__

0741fc6

targets: passes.py -> misc.py

c53cd79

operator: Refactor lowering methods

b1212ae

Generalize Targets as Operators [pass 0]

ab17d9e

Generalize Targets as Operators [pass 1]

61c1487

Generalize Targets as Operators [pass 2]

2cb0135

Generalize Targets as Operators [pass 3]

7a743d3

Generalize Targets as Operators [pass 4]

b817978

yask: Yask -> YASK all over the place

41ab3bd

target_pass -> iet_pass

77b7c50

tests: Drop some dle=noop so that YASK behaves

d68ca3c

passes: Flush denormals once, not in each efunc

c73ca8f

Fixup docstring

82a4304

yask: Fixup noop operators

1c09e0b

Clean up: whitespaces, import ordering, docstrings ...

6a31820

FabioLuporini force-pushed the refactor-as-operators branch from fec5d95 to 6a31820 Compare January 15, 2020 13:14

georgebisbas reviewed Jan 15, 2020

View reviewed changes

mloubout reviewed Jan 15, 2020

View reviewed changes

Fixup comments and docstrings

e33e6a3

FabioLuporini commented Jan 16, 2020

View reviewed changes

georgebisbas reviewed Jan 16, 2020

View reviewed changes

mloubout approved these changes Jan 16, 2020

View reviewed changes

FabioLuporini merged commit 14b475c into master Jan 16, 2020

FabioLuporini deleted the refactor-as-operators branch January 16, 2020 17:25

Generalize Targets as Operators #1028

Generalize Targets as Operators #1028

Conversation

FabioLuporini commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2020 • edited

Codecov Report

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini commented Jan 16, 2020

codecov bot commented Jan 8, 2020 •

edited

FabioLuporini Jan 16, 2020 •

edited