Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Misc compiler tweaks and improvements #2136

Merged
merged 18 commits into from
May 31, 2023
Merged

Conversation

FabioLuporini
Copy link
Contributor

Context: essentially all of this is necessary for new features and bug fixes in PRO

I'd like to start getting reviews ASAP because we gonna need to merge this rapidly

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #2136 (45ddbcb) into master (c7d15b6) will decrease coverage by 0.09%.
The diff coverage is 82.06%.

@@            Coverage Diff             @@
##           master    #2136      +/-   ##
==========================================
- Coverage   87.17%   87.09%   -0.09%     
==========================================
  Files         222      222              
  Lines       39383    39523     +140     
  Branches     5121     5127       +6     
==========================================
+ Hits        34334    34424      +90     
- Misses       4478     4521      +43     
- Partials      571      578       +7     
Impacted Files Coverage Δ
devito/operator/profiling.py 68.76% <0.00%> (ø)
devito/passes/clusters/asynchrony.py 11.64% <0.00%> (ø)
devito/passes/iet/parpragma.py 88.52% <0.00%> (-0.49%) ⬇️
tests/test_gpu_common.py 1.37% <5.00%> (+0.05%) ⬆️
devito/ir/support/properties.py 80.00% <33.33%> (-1.31%) ⬇️
devito/passes/iet/asynchrony.py 13.33% <33.33%> (+0.62%) ⬆️
devito/core/operator.py 85.99% <60.00%> (+0.13%) ⬆️
devito/passes/iet/definitions.py 82.10% <66.66%> (-0.21%) ⬇️
devito/passes/clusters/buffering.py 92.06% <90.24%> (-1.70%) ⬇️
devito/passes/iet/mpi.py 93.03% <90.90%> (-3.17%) ⬇️
... and 15 more

... and 2 files with indirect coverage changes

devito/ir/stree/algorithms.py Show resolved Hide resolved
devito/ir/stree/algorithms.py Show resolved Hide resolved
devito/passes/clusters/buffering.py Show resolved Hide resolved
@@ -371,14 +361,14 @@ def __init__(self, function, d, accessv, options, sregistry):
else:
size = async_degree

# Replace `d` with a suitable CustomDimension `bd`
# Create `bd` -- a contraction Dimension for `dim`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid bd?
Cuz we use the same name in blocking?
We would not like to grep bd and get both contexts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, changing to xd (cd is taken too...)

# six slots, used to replace a buffered Function accessed at `d-3`, `d`
# and `d + 2`, will have `offset = 3`
p, offset = offset_from_centre(d, indices)
# The buffer is initialized at `d_m(d_M) - offset`. E.g., a buffer with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not say that this is clear

devito/passes/iet/definitions.py Show resolved Hide resolved
@@ -1094,6 +1097,36 @@ def test_streaming_split_noleak(self):
assert np.all(u.data[0] == u1.data[0])
assert np.all(u.data[1] == u1.data[1])

@pytest.mark.skip(reason="Unsupported MPI + .dx when streaming backwards")
@pytest.mark.parallel(mode=4)
@switchconfig(safe_math=True) # Or NVC will crash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not try the condition logic here from my PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it later on once it's merged (haven't reviewed it in its last form yet)

continue

dims = b.function.dimensions
lhs = b.indexed[[b.initmap.get(d, Map(d, d)).b for d in dims]]
rhs = b.function[[b.initmap.get(d, Map(d, d)).f for d in dims]]
lhs = b.indexed[dims].subs(dim, b.initidx.b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_subs more efficient for single sub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

lhs = b.indexed[[b.lastmap.get(d, Map(d, d)).b for d in dims]]
rhs = b.function[[b.lastmap.get(d, Map(d, d)).f for d in dims]]
lhs = b.indexed[dims].subs(b.dim, b.lastidx.b)
rhs = b.function[dims].subs(b.dim, b.lastidx.f)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


@cached_property
def initmap(self):
def initidx(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe first to match last

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@FabioLuporini FabioLuporini merged commit 51e11e5 into master May 31, 2023
@FabioLuporini FabioLuporini deleted the async-compr-cpu branch May 31, 2023 07:02
@FabioLuporini
Copy link
Contributor Author

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants