compiler: Fix data dependency issue on dimension-wide WAR #2200

georgebisbas · 2023-09-08T11:33:28Z

No description provided.

codecov · 2023-09-08T11:39:20Z

Codecov Report

Merging #2200 (fc57d84) into master (5eaad80) will decrease coverage by 24.19%.
The diff coverage is 96.15%.

@@             Coverage Diff             @@
##           master    #2200       +/-   ##
===========================================
- Coverage   87.08%   62.90%   -24.19%     
===========================================
  Files         228      144       -84     
  Lines       40611    22716    -17895     
  Branches     7429     4143     -3286     
===========================================
- Hits        35367    14290    -21077     
- Misses       4644     7694     +3050     
- Partials      600      732      +132

Files Changed	Coverage Δ
devito/ir/support/basic.py	`84.58% <80.00%> (-8.29%)`	⬇️
tests/test_operator.py	`96.41% <100.00%> (-1.33%)`	⬇️

... and 192 files with indirect coverage changes

FabioLuporini

Looking good to me!

Minors: can you rebase, merge together squashable commits, rename them to honor our commit message rules (@georgebisbas can explain and point to the right documentation?

Also (another nitpick), I'm incrementally trying to shorten our comments and to wrap them around the 80th(ish) character, while the previous limit wash 90 characters long rows. While this is still loosely applied, it would help to refactor the comments to keep this mind. Also, as I said, the more compact, the better. And, final nitpick: if a comment belongs to e.g. the body of an if, it should go inside the if, not outside

FabioLuporini · 2023-09-09T08:25:12Z

devito/ir/support/basic.py

+                # For example:
+                # Eq(u[0, y], 1)
+                # Eq(u[1, y+1], u[0, 1])
+                elif i.is_Number and not j.is_Number:


instead of not j.is_Number you can also use j.is_Symbol, they should be equivalent in this context

also you must cover the symmetric case:

elif (i.is_Number and j.is_Symbol) or (i.is_Symbol and j.is_Number):

(parametrize test?)

instead of not j.is_Number you can also use j.is_Symbol, they should be equivalent in this context

also you must cover the symmetric case:

elif (i.is_Number and j.is_Symbol) or (i.is_Symbol and j.is_Number):

For this George and I figured that we only need i.is_Number and j.is_Symbol case since we would only have a dependency if we are reading from one location and writing over an entire dimension. The other case would have been caught previously.

Consider the equations

eq0 = Eq(u[0,y], 1) eq1 = Eq(u[1,1], u[0, y+1])

The dependency between reading from u[0, y+1] and writing to u[0,y] is caught by

v = i - j if v.is_Number and v.is_finite: # If i and j are numbers, we append an (I) distance if i.is_Number and j.is_Number: return Vector(S.ImaginaryUnit) # If both i and j are not numbers, # there may be dimension-dependent dependencies # so we append the distance else: ret.append(v)

The dependency between reading from u[0,y+1] and writing to u[1,1] is imaginary. Hence the algorithm doesn't append anything to the distance vector

I'm not so particularly convinced we shouldn't care about the symmetrict case

what if you have

Eq(u[0, 1], 1) Eq(u[x, y], u[0, y])

?

Also:

you're still using "not j.is_Number", instead of "is_Symbol"

how is your solution any stronger and/or more robust than just having

if i.is_Symbol or j.is_Symbol: ..

FabioLuporini · 2023-09-09T08:26:26Z

devito/ir/support/basic.py

+                    # If both i and j are not numbers, there may be dimension-dependent
+                    # dependencies so we append the distance
+                    else:
+                        ret.append(v)


we might want to append Infinite here, otherwise I think we would have never ended up in this place, because we would have never left the first big loop at the top of the method over the iteration intervals

The equation pair

eq0 = Eq(u[0,y], 1) eq1 = Eq(u[1,1], u[0, y+1])

would resulting in the execution ending up at line 435

Also I'm confused to why adding an Infinite here would help with never reaching this part of the code

FabioLuporini

History requires rebase and squashing

commit messages do not adhere to our format. Please see the contributing guidelines

FabioLuporini · 2023-09-14T07:13:32Z

tests/test_ir.py

@@ -826,6 +826,61 @@ def test_dep_nasty(self, eqns):
        scope = Scope(eqns)
        assert len(scope.d_all) == 1

+    def test_2194(self):


if u are performing numerical evaluation, you should rather move these tests to TestOperator::TestLoopScheduling

You could also slightly tweak them to rather have one single macro-test with pytest.parametrize(exprs, expected) just like we do with several other tests

FabioLuporini · 2023-09-14T07:15:01Z

tests/test_operator.py

@@ -1933,7 +1933,65 @@ def test_topofuse_w_numeric_dim(self):
        op = Operator(eqns)

        assert_structure(op, ['r,i', 'r'], 'r,i')
+
+    def test_2194(self):


what's the difference between these tests and the ones before?

they look redundant to me at first glance?

in fact, these seem to be a superset of those in test_ir, by also checking the structure of the resulting loop nest

if that's the case:

compact them as a single test + py.test.parametrize

drop the redundant tests in test_ir

devito/ir/support/basic.py

FabioLuporini · 2023-09-14T07:16:11Z

devito/ir/support/basic.py

+                    # If i and j are numbers, we append an (I) distance
+                    if i.is_Number and j.is_Number:
+                        return Vector(S.ImaginaryUnit)
+                    # If both i and j are not numbers, there may be dimension-dependent


this is also another useless comment, maybe we can instead use a tiny E.g.

possibly inside the else, not on the outside...

FabioLuporini · 2023-09-14T07:23:22Z

devito/ir/support/basic.py

+                # For example:
+                # Eq(u[0, y], 1)
+                # Eq(u[1, y+1], u[0, 1])
+                elif i.is_Number and not j.is_Number:


I'm not so particularly convinced we shouldn't care about the symmetrict case

what if you have

Eq(u[0, 1], 1) Eq(u[x, y], u[0, y])

?

Also:

you're still using "not j.is_Number", instead of "is_Symbol"

how is your solution any stronger and/or more robust than just having

if i.is_Symbol or j.is_Symbol: ..

FabioLuporini · 2023-09-14T07:24:12Z

Also: pep8 isn't honored. See failing build

FabioLuporini

Left a few more comments

Also, given the nature and the size of the edits, I suggest to squash all of the 5 commits into a single commit with message "compiler: Patch ..."

FabioLuporini · 2023-09-20T12:14:47Z

devito/ir/support/basic.py

+
+                # We are writing over an entire dimension
+                # but reading from one point.
+                # If there are overlaps between the two


Drop this entire paragraph (starting with "If there are overlaps...")

FabioLuporini · 2023-09-20T12:15:24Z

devito/ir/support/basic.py

+                        # Eq(u[1, 1], u[0, y+1])
+                        ret.append(S.Infinity)
+
+                # We are writing over an entire dimension


you're breaking to a new line too soon. Use around 83~84 characters, then break line

also simplify as "Writing (reading) over an entire dimension, reading (writing) from one point"

FabioLuporini · 2023-09-20T12:15:47Z

devito/ir/support/basic.py

+                    if i.is_Number and j.is_Number:
+                        return Vector(S.ImaginaryUnit)
+                    else:
+                        # If both i and j are not numbers,


Drop this paragraph

devito/ir/support/basic.py

compiler: Addressed comments on PR #2200

Co-Authored-By: AndrewCheng827 <ac1721.ac.uk>

georgebisbas · 2023-09-20T16:39:51Z

Closing in favour of #2212

georgebisbas added the compiler label Sep 8, 2023

georgebisbas self-assigned this Sep 8, 2023

georgebisbas linked an issue Sep 8, 2023 that may be closed by this pull request

Faulty Data Dependence Analysis #2194

Closed

georgebisbas changed the title ~~compiler: Fix data dependency issue on dimension-wide WAR close 2194~~ compiler: Fix data dependency issue on dimension-wide WAR Sep 8, 2023

FabioLuporini reviewed Sep 9, 2023

View reviewed changes

FabioLuporini requested changes Sep 14, 2023

View reviewed changes

georgebisbas force-pushed the fix_2194 branch from 26df87c to 482936d Compare September 14, 2023 13:49

FabioLuporini requested changes Sep 20, 2023

View reviewed changes

georgebisbas added a commit that referenced this pull request Sep 20, 2023

Merge pull request #2211 from AndrewCheng827/Andrew_fix_2194

1ded3da

compiler: Addressed comments on PR #2200

georgebisbas force-pushed the fix_2194 branch from 1ded3da to 76aac12 Compare September 20, 2023 16:08

compiler: Fix issue 2194

fc57d84

Co-Authored-By: AndrewCheng827 <ac1721.ac.uk>

georgebisbas force-pushed the fix_2194 branch from 76aac12 to fc57d84 Compare September 20, 2023 16:17

georgebisbas closed this Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Fix data dependency issue on dimension-wide WAR #2200

compiler: Fix data dependency issue on dimension-wide WAR #2200

georgebisbas commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

FabioLuporini left a comment

FabioLuporini Sep 9, 2023

FabioLuporini Sep 9, 2023

AndrewCheng827 Sep 13, 2023

FabioLuporini Sep 14, 2023

FabioLuporini Sep 9, 2023

AndrewCheng827 Sep 13, 2023

FabioLuporini left a comment

FabioLuporini Sep 14, 2023

FabioLuporini Sep 14, 2023

FabioLuporini Sep 14, 2023

FabioLuporini Sep 14, 2023

FabioLuporini Sep 14, 2023

FabioLuporini commented Sep 14, 2023

FabioLuporini left a comment

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

FabioLuporini Sep 20, 2023

georgebisbas commented Sep 20, 2023

compiler: Fix data dependency issue on dimension-wide WAR #2200

compiler: Fix data dependency issue on dimension-wide WAR #2200

Conversation

georgebisbas commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

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

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

FabioLuporini commented Sep 14, 2023

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

georgebisbas commented Sep 20, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading