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: Improve quality of generated code #2263

Merged
merged 18 commits into from
Nov 21, 2023
Merged

compiler: Improve quality of generated code #2263

merged 18 commits into from
Nov 21, 2023

Conversation

FabioLuporini
Copy link
Contributor

This PR, and its sibling in PRO, dramatically improve the quality of the generated code when running production-level kernels. This is mostly evident in PRO, when compression, serialization, mpi, and data streaming are all used in combination in the same Operator. A lot of code is now nicely abstracted away into routines, which significantly decreases the final code size.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Attention: 181 lines in your changes are missing coverage. Please review.

Comparison is base (21ef56c) 86.91% compared to head (4dfc228) 86.80%.

Files Patch % Lines
tests/test_gpu_common.py 0.00% 74 Missing ⚠️
devito/passes/iet/asynchrony.py 7.69% 36 Missing ⚠️
devito/passes/iet/engine.py 61.97% 23 Missing and 4 partials ⚠️
devito/ir/iet/visitors.py 69.04% 9 Missing and 4 partials ⚠️
devito/ir/iet/efunc.py 59.09% 9 Missing ⚠️
devito/passes/iet/orchestration.py 0.00% 8 Missing ⚠️
devito/passes/iet/languages/openacc.py 44.44% 4 Missing and 1 partial ⚠️
devito/core/gpu.py 60.00% 2 Missing ⚠️
devito/passes/__init__.py 50.00% 1 Missing and 1 partial ⚠️
devito/symbolics/printer.py 33.33% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2263      +/-   ##
==========================================
- Coverage   86.91%   86.80%   -0.12%     
==========================================
  Files         229      229              
  Lines       42156    42296     +140     
  Branches     7787     7830      +43     
==========================================
+ Hits        36641    36715      +74     
- Misses       4865     4921      +56     
- Partials      650      660      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1253,3 +1281,16 @@ def generate(self):
if self.cast:
tip = '(%s)%s' % (self.cast, tip)
yield tip


def sorted_efuncs(efuncs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be simpler to define that in efunc to avoid the local import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

circular import, can't import efunc from visitors

v = priority[cls]
break
else:
v = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

WHy no just a priority.get(i.__class__, 0)

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 it's here it's because we can't infer anything from priority?

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean instead of the whole for/else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see. Basically, simply because we want to hit subclasses too, which is essentially the point of this utility

@@ -36,7 +36,7 @@ def __init__(self, *functions, op=dv.mpi.MPI.SUM, dtype=None):
def __enter__(self):
i = dv.Dimension(name='mri',)
self.n = dv.Function(name='n', shape=(1,), dimensions=(i,),
grid=self.grid, dtype=self.dtype)
grid=self.grid, dtype=self.dtype, space='host')
Copy link
Contributor

Choose a reason for hiding this comment

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

why not local?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because this one really needs to be on the host as it will ultimately be accessed in user-land

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not exceptionally elegant, admittedly

# Attempt simplifcation
# E.g., `FLOAT(32) -> 32.0` of type `sympy.Float`
try:
return sympify(eval(cls._base_typ)(base))
Copy link
Contributor

Choose a reason for hiding this comment

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

THis will loose accuracy for double but not sure what's the easier way as there is only float and int really defined as python types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no unless there's an 'F' in C all numbers with digits are double, so we should be good

@mloubout mloubout merged commit 7a86b36 into master Nov 21, 2023
31 checks passed
@mloubout mloubout deleted the better-codegen branch November 21, 2023 16:31
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.

2 participants