compiler: Patch race conditions due to storage-related dependencies #1903

FabioLuporini · 2022-04-12T12:29:03Z

fixes #1901
fixes #1902

codecov · 2022-04-12T12:33:52Z

Codecov Report

Merging #1903 (f55e68a) into master (30434ef) will increase coverage by 1.29%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #1903      +/-   ##
==========================================
+ Coverage   88.35%   89.64%   +1.29%     
==========================================
  Files         210      210              
  Lines       35460    35523      +63     
  Branches     5350     5361      +11     
==========================================
+ Hits        31329    31843     +514     
+ Misses       3643     3183     -460     
- Partials      488      497       +9

Impacted Files	Coverage Δ
devito/ir/clusters/algorithms.py	`95.30% <ø> (ø)`
devito/passes/clusters/asynchrony.py	`37.33% <0.00%> (+23.99%)`	⬆️
devito/arch/compiler.py	`54.59% <75.00%> (+3.00%)`	⬆️
tests/test_dle.py	`97.12% <90.00%> (-1.01%)`	⬇️
devito/passes/iet/parpragma.py	`90.13% <94.11%> (+0.67%)`	⬆️
devito/core/cpu.py	`100.00% <100.00%> (ø)`
devito/core/gpu.py	`95.63% <100.00%> (+4.99%)`	⬆️
devito/ir/support/basic.py	`92.41% <100.00%> (+0.49%)`	⬆️
devito/mpi/halo_scheme.py	`97.31% <100.00%> (+0.04%)`	⬆️
devito/passes/clusters/buffering.py	`94.88% <100.00%> (+0.13%)`	⬆️
... and 28 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 30434ef...f55e68a. Read the comment docs.

mloubout

Minor comments, but looks GTG

devito/passes/clusters/buffering.py

mloubout · 2022-04-12T12:33:44Z

tests/test_buffering.py

+    usave = TimeFunction(name='usave', grid=grid, save=10)
+    v = TimeFunction(name='v', grid=grid)
+
+    eq = [Eq(v[time, x, y], usave)]


nitpicking: Eq(usave, v) bit more readable and makes bit more "sense"

I want time in place of t since otherwise the loop will be tagged SEQUENTIAL due to the presence of uindices. But I want it fully PARALLEL to trigger the issue

Yeah that's fin,e I meant having usave as lhs mostly but again just nitpicking

ah, I see what you mean now...
well in backward propagators you do have usave's on the RHS :)

mloubout · 2022-04-12T13:48:52Z

devito/core/cpu.py

@@ -143,14 +143,15 @@ class Cpu64NoopOperator(Cpu64OperatorMixin, CoreOperator):
    def _specialize_iet(cls, graph, **kwargs):
        options = kwargs['options']
        platform = kwargs['platform']
+        compiler = kwargs['compiler']


Note: not for here but long run, should be gather all of these into a CompilerOptions class to avoid carrying all these multiple arguments everywhere.

that's a fair point, but I actually don't mind supplying the passes only the strict necessary rather than a big batch of things

mloubout · 2022-04-12T13:51:05Z

devito/passes/iet/parpragma.py

+        def supported():
+            # Not all backend compilers support array reduction!
+            # Here are the known unsupported ones:
+            if isinstance(self.compiler, GNUCompiler) and \


SHould we havd something similar for osx clang + openmp?

ah, yeah, probably..

georgebisbas · 2022-04-12T17:43:39Z

devito/ir/support/basic.py

+    @memoized_meth
+    def is_const(self, dim):
+        """
+        True if a constant depedence, that is no Dimensions involved, False otherwise.


typo depedence

georgebisbas · 2022-04-13T09:53:39Z

tests/test_dle.py

-        if simd_level:
-            assert 'omp simd' in iterations[simd_level].pragmas[0].value
+        try:
+            assert 'omp for collapse' in iterations[0].pragmas[0].value


Does that mean that you do not see "omp for collapse" in every case as before?
If this is the case I would prefer another variable in parametrization to whether "omp for collapse" is expected or not.
If we still see "omp for collapse" it should be out of the try-except.

Ideally we could use parameters to avoid the try-except. THese parameters would reflect the expected positions of omp and simd pragmas.

false, it is caught in the except

Then why do we not have:

assert 'omp for collapse' in iterations[0].pragmas[0].value try: if simd_level: assert 'omp simd' in iterations[simd_level].pragmas[0].value except: # E.g. gcc-5 doesn't support array reductions, so the compiler will # generate different legal code assert not Ompizer._support_array_reduction(configuration['compiler'])

The "omp for collapse is always in iterations[0], right?

If it just false we can add a similar var to simd_level, like omp_level to get either 0 or None...
i.e.

if omp_level: assert 'omp for collapse' in iterations[0].pragmas[0].value

The "omp for collapse is always in iterations[0], right?

no, with gcc-5 it will be in the innermost loop*, as it prefers thread parallelism over simd parallelism

it's a bit more complicated than that... it actually depends on the number of physical and logical cores of the underlying platform, as collapsing may further change what the candidate-selector ends up choosing. Hence the loose(ish) any(...) in the except branch

georgebisbas

GTG from me once green!

georgebisbas · 2022-04-13T12:14:30Z

tests/test_dle.py

-        if simd_level:
-            assert 'omp simd' in iterations[simd_level].pragmas[0].value
+        try:
+            assert 'omp for collapse' in iterations[0].pragmas[0].value


compiler: Patch race conditions due to storage-related dependencies

57b6ca3

FabioLuporini added the bug-C bug in the generated code label Apr 12, 2022

FabioLuporini requested review from mloubout and georgebisbas April 12, 2022 12:29

mloubout approved these changes Apr 12, 2022

View reviewed changes

mloubout reviewed Apr 12, 2022

View reviewed changes

mloubout approved these changes Apr 12, 2022

View reviewed changes

FabioLuporini force-pushed the fix-issues-1901-1696 branch from 73856fc to 9b5c3d7 Compare April 12, 2022 15:24

FabioLuporini added 2 commits April 12, 2022 17:43

compiler: Avoid omp array reductions with gcc5 as unsupported

9912123

arch: Enable NVC on Host platforms

7681225

FabioLuporini force-pushed the fix-issues-1901-1696 branch from 9b5c3d7 to 7681225 Compare April 12, 2022 15:44

georgebisbas reviewed Apr 12, 2022

View reviewed changes

FabioLuporini added 2 commits April 13, 2022 09:12

misc: Fix typos

0781498

compiler: Avoid nesting unsuppored simd+atomic pragmas

f55e68a

FabioLuporini force-pushed the fix-issues-1901-1696 branch from 870fcaa to f55e68a Compare April 13, 2022 09:44

georgebisbas reviewed Apr 13, 2022

View reviewed changes

georgebisbas approved these changes Apr 13, 2022

View reviewed changes

mloubout approved these changes Apr 13, 2022

View reviewed changes

FabioLuporini merged commit e1b1692 into master Apr 13, 2022

FabioLuporini deleted the fix-issues-1901-1696 branch April 13, 2022 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Patch race conditions due to storage-related dependencies #1903

compiler: Patch race conditions due to storage-related dependencies #1903

FabioLuporini commented Apr 12, 2022

codecov bot commented Apr 12, 2022 •

edited

Loading

mloubout left a comment

mloubout Apr 12, 2022

FabioLuporini Apr 12, 2022

mloubout Apr 12, 2022

FabioLuporini Apr 12, 2022

mloubout Apr 12, 2022

FabioLuporini Apr 12, 2022

mloubout Apr 12, 2022

FabioLuporini Apr 12, 2022

georgebisbas Apr 12, 2022

FabioLuporini Apr 13, 2022

georgebisbas Apr 13, 2022

FabioLuporini Apr 13, 2022

georgebisbas Apr 13, 2022

georgebisbas Apr 13, 2022

FabioLuporini Apr 13, 2022

georgebisbas Apr 13, 2022

georgebisbas left a comment

georgebisbas Apr 13, 2022

compiler: Patch race conditions due to storage-related dependencies #1903

compiler: Patch race conditions due to storage-related dependencies #1903

Conversation

FabioLuporini commented Apr 12, 2022

codecov bot commented Apr 12, 2022 • edited Loading

Codecov Report

mloubout 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

georgebisbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2022 •

edited

Loading