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: Add fission-for-pressure subpass #2114

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

FabioLuporini
Copy link
Contributor

So far, this isn't enabled by default. More experimentation required

This PR also adds a new compiler flag to GNUCompiler when running on Skylake and later processors. I've tested it in a few places, and it seems to generally improve things. I'd like people to reproduce it. Note that you need to ensure DEVITO_PLATFORM is properly set to skx, clk, ... the autodetection mechanism, with the recent proliferation of intel architectures, doesn't seem to be as good as it was.

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #2114 (6ef0b41) into master (39af05b) will decrease coverage by 25.81%.
The diff coverage is 23.50%.

@@             Coverage Diff             @@
##           master    #2114       +/-   ##
===========================================
- Coverage   87.77%   61.96%   -25.81%     
===========================================
  Files         221      222        +1     
  Lines       39003    39107      +104     
  Branches     5067     5081       +14     
===========================================
- Hits        34233    24232    -10001     
- Misses       4208    14163     +9955     
- Partials      562      712      +150     
Impacted Files Coverage Δ
tests/test_fission.py 13.88% <11.42%> (-86.12%) ⬇️
devito/passes/clusters/fission.py 17.64% <17.64%> (ø)
devito/arch/compiler.py 40.55% <50.00%> (-0.52%) ⬇️
devito/arch/archinfo.py 42.46% <100.00%> (+0.40%) ⬆️
devito/core/cpu.py 93.50% <100.00%> (-6.50%) ⬇️
devito/core/gpu.py 74.07% <100.00%> (+0.24%) ⬆️
devito/core/operator.py 70.81% <100.00%> (-15.05%) ⬇️
devito/passes/clusters/__init__.py 100.00% <100.00%> (ø)
devito/passes/clusters/misc.py 78.30% <100.00%> (-11.57%) ⬇️

... and 129 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

continue

# At this point we know we want to fission. But can we?
for dep in c.scope.d_flow.independent():
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 check this first and avoid all the retrieve/sets/...

ns = len(functions_shared)

if not ns:
ns = .001
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess safe to assume we won't have over 1k function

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some comment for this, even though not really necessary? Just to justify this option

ispace = c.ispace.lift(d)
processed.append(c.rebuild(exprs=g1, ispace=ispace))

break
Copy link
Contributor

Choose a reason for hiding this comment

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

So at most one fission? Could check the remainder g can be split too not sure of what would want

processed.append(c.rebuild(exprs=g0))
processed.append(fission_for_pressure(c.rebuild(exprs=g1, ispace=ispace)))

pass


class IntelGoldenCode(Intel64):
Copy link
Contributor

Choose a reason for hiding this comment

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

GoldenCove?

ns = len(functions_shared)

if not ns:
ns = .001
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add some comment for this, even though not really necessary? Just to justify this option

t = grid.stepping_dim
def test_nofission_as_unprofitable(self):
"""
Test there's no fission if not gonna increase number of collapsable loops.
Copy link
Contributor

Choose a reason for hiding this comment

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

"gonna" -> "going to"?

assert_structure(op, ['t,x,y', 't,x,y'], 't,x,y,y')
def test_fission_partial(self):
"""
Test there's no fission if not gonna increase number of collapsable loops.
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment

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.

None yet

4 participants