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: Introduce symbolic fencing #2244

Merged
merged 12 commits into from
Oct 25, 2023
Merged

Conversation

FabioLuporini
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #2244 (a000c49) into master (ae40de0) will decrease coverage by 0.03%.
The diff coverage is 79.72%.

@@            Coverage Diff             @@
##           master    #2244      +/-   ##
==========================================
- Coverage   86.91%   86.89%   -0.03%     
==========================================
  Files         229      229              
  Lines       41573    41687     +114     
  Branches     7665     7695      +30     
==========================================
+ Hits        36133    36223      +90     
- Misses       4818     4835      +17     
- Partials      622      629       +7     
Files Coverage Δ
devito/ir/clusters/visitors.py 90.08% <100.00%> (ø)
devito/types/parallel.py 82.20% <100.00%> (ø)
devito/ir/support/basic.py 91.66% <96.15%> (+0.04%) ⬆️
devito/passes/clusters/misc.py 89.49% <66.66%> (+0.04%) ⬆️
devito/types/misc.py 97.00% <95.00%> (-0.57%) ⬇️
tests/test_ir.py 94.81% <97.05%> (+0.27%) ⬆️
devito/ir/clusters/cluster.py 93.33% <80.00%> (-0.81%) ⬇️
devito/ir/stree/algorithms.py 89.24% <0.00%> (-2.32%) ⬇️
devito/passes/clusters/asynchrony.py 12.04% <20.00%> (-0.26%) ⬇️
devito/passes/clusters/utils.py 75.00% <51.85%> (-21.78%) ⬇️

... and 1 file with indirect coverage changes

@georgebisbas
Copy link
Contributor

What is the overarching target of this PR?

all(a.is_regular for a in self.scope.accesses))

@cached_property
def is_sparse(self):
"""
A Cluster is sparse if it represents a sparse operation, i.e iff
There's at least one irregular access.
True if it represents a sparse operation, i.e iff there's at least
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra f on "iff"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if and only if?

# Synchronization operations prevent lifting
if c.syncs.get(dim):
# Synchronization prevents lifting
if c.syncs.get(dim) or \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this get too long as one line?

Copy link
Contributor Author

@FabioLuporini FabioLuporini Oct 24, 2023

Choose a reason for hiding this comment

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

I do the splitting on purpose for clarity when I feel there's (relatively speaking) little correlation between the two operands of the relation

@FabioLuporini
Copy link
Contributor Author

@georgebisbas

What is the overarching target of this PR?

Necessary tweaks for stuff in PRO

@FabioLuporini
Copy link
Contributor Author

@georgebisbas

basically it provides a series of mechanisms to impose that given subsequences of Clusters don't float around upon topological sorting, even if deemed possible by data dependence analysis

Clusters sitckiness helps with:

  • performance (tough to explain though)
  • code readability
  • development itself ! because it in essence creates an atomic sequence of Clusters, which is much simpler to handle in the case of asynchronous operations

I'm not sure how to explain this any better without showing the actual low-level details

@FabioLuporini FabioLuporini merged commit 522d475 into master Oct 25, 2023
32 checks passed
@FabioLuporini FabioLuporini deleted the patch-lazy-threading branch October 25, 2023 09:09
@georgebisbas
Copy link
Contributor

@georgebisbas

basically it provides a series of mechanisms to impose that given subsequences of Clusters don't float around upon topological sorting, even if deemed possible by data dependence analysis

Clusters sitckiness helps with:

  • performance (tough to explain though)
  • code readability
  • development itself ! because it in essence creates an atomic sequence of Clusters, which is much simpler to handle in the case of asynchronous operations

I'm not sure how to explain this any better without showing the actual low-level details

Thanks, these things could easily get into description to help review.

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