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.

Sign up for GitHub

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

Jump to bottom

arch: Add ICX support #2051

Merged
merged 15 commits into from
Jun 6, 2023
Merged

arch: Add ICX support #2051

merged 15 commits into from
Jun 6, 2023

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Jan 23, 2023 •
edited
Loading

@georgebisbas georgebisbas self-assigned this Jan 23, 2023
@codecov
Copy link

codecov bot commented Jan 23, 2023 •
edited
Loading

Codecov Report

Merging #2051 (e355ed8) into master (5f1ff64) will decrease coverage by 0.06%.
The diff coverage is 35.82%.

@@            Coverage Diff             @@
##           master    #2051      +/-   ##
==========================================
- Coverage   87.14%   87.09%   -0.06%     
==========================================
  Files         223      223              
  Lines       39647    39686      +39     
  Branches     5139     5147       +8     
==========================================
+ Hits        34550    34564      +14     
- Misses       4521     4544      +23     
- Partials      576      578       +2
Impacted Files Coverage Δ
tests/test_buffering.py 98.70% <ø> (-0.01%) ⬇️
devito/arch/compiler.py 39.34% <14.89%> (-1.44%) ⬇️
tests/conftest.py 84.86% <25.00%> (-1.40%) ⬇️
devito/arch/archinfo.py 42.31% <100.00%> (+0.61%) ⬆️
devito/parameters.py 85.81% <100.00%> (+0.20%) ⬆️
tests/test_benchmark.py 87.23% <100.00%> (+0.56%) ⬆️
tests/test_dimension.py 100.00% <100.00%> (ø)

mloubout
mloubout reviewed Jan 23, 2023
View reviewed changes
devito/arch/compiler.py Outdated Show resolved Hide resolved
mloubout
mloubout reviewed Feb 17, 2023
View reviewed changes
Copy link
Contributor

@mloubout mloubout left a comment

Choose a reason for hiding this comment

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

Could we at least add it to the icc-cpu docker CI

@georgebisbas georgebisbas force-pushed the add_icx_support branch 2 times, most recently from 1ffff27 to 70f6190 Compare March 8, 2023 12:59
@georgebisbas georgebisbas force-pushed the add_icx_support branch 2 times, most recently from 84e1292 to a5f56cb Compare March 17, 2023 18:48
mloubout
mloubout reviewed Mar 24, 2023
View reviewed changes
Copy link
Contributor

@mloubout mloubout left a comment

Choose a reason for hiding this comment

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

I still don't like having separate OneApi and Intel compiler

docker/Dockerfile.cpu Outdated
##############################################################
FROM base as icx

# Download the key to system keyring
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole part is a waste , just do FROM icc as icx and just set the DEVITO env vars

tests/conftest.py Outdated
@@ -69,6 +71,12 @@ def skipif(items, whole_module=False):
isinstance(configuration['platform'], Cpu64):
skipit = "`icc+cpu64` won't work with this test"
break
# Skip if it won't run with OneAPICompiler
if i == 'cpu64-icpx' and \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need that, if everything is a proper IntelCompiler then we just need cpu64-intel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do that, but we have tests that, at least temporarily, pass with ICC but not with ICX.
What you propose should be fine with the next release and at some point, we are gonna drop ICC entirely.
I do not have any string preference

FabioLuporini
FabioLuporini reviewed Mar 30, 2023
View reviewed changes
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

Let's try taking this to completion

devito/arch/compiler.py Outdated Show resolved Hide resolved
tests/test_dimension.py Outdated Show resolved Hide resolved
@georgebisbas georgebisbas force-pushed the add_icx_support branch 2 times, most recently from 08f7e1c to 6d717fd Compare March 30, 2023 11:05
@georgebisbas georgebisbas mentioned this pull request Mar 31, 2023
Closed
georgebisbas
georgebisbas commented Apr 3, 2023
View reviewed changes
.github/workflows/pytest-core-nompi.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename to intel-icc, intel-icx?

Copy link
Contributor

Choose a reason for hiding this comment

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

just icc or icx ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

οκ

FabioLuporini
FabioLuporini requested changes Apr 4, 2023
View reviewed changes
.github/workflows/pytest-core-nompi.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

just icc or icx ?

.github/workflows/pytest-core-nompi.yml Outdated Show resolved Hide resolved
.github/workflows/pytest-core-nompi.yml Outdated Show resolved Hide resolved
devito/arch/compiler.py Outdated
@@ -793,6 +847,8 @@ def __lookup_cmds__(self):
'intel': IntelCompiler,
'icpc': IntelCompiler,
'icc': IntelCompiler,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether icc and intel should default to OpenapiCompiler as well. I think they do. At least DEVITO_ARCH=intel.

icc maybe the only one that really should attempt to pick up icc

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

tests/test_dimension.py Outdated Show resolved Hide resolved
tests/test_dimension.py Outdated Show resolved Hide resolved
devito/parameters.py Outdated Show resolved Hide resolved
@georgebisbas georgebisbas force-pushed the add_icx_support branch 2 times, most recently from 0202e68 to 7a269dd Compare April 7, 2023 22:45
@georgebisbas georgebisbas force-pushed the add_icx_support branch 2 times, most recently from d2b4c86 to df590b7 Compare April 19, 2023 14:00
FabioLuporini
FabioLuporini reviewed Jun 4, 2023
View reviewed changes
devito/arch/compiler.py Outdated

language = kwargs.pop('language', configuration['language'])
self.cflags.append("-xHost")
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally this line goes after pulling platform and language, but we can address it in another PR if it can spare a useless CI spin

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

FabioLuporini
FabioLuporini reviewed Jun 4, 2023
View reviewed changes
devito/arch/compiler.py Outdated
self.cflags.append('-fdebug-info-for-profiling')

# Make sure the MPI compiler uses `icx` underneath -- whatever the MPI distro is
if kwargs.get('mpi'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, can we now not drop this? Why was the conversation marked as "resolved" ? We should now be getting the MPI warning via super()?

FabioLuporini
FabioLuporini reviewed Jun 4, 2023
View reviewed changes
tests/test_buffering.py
@@ -701,7 +701,6 @@ def test_everything():
assert np.all(u.data == u1.data)


@skipif('cpu64-icc')
Copy link
Contributor

Choose a reason for hiding this comment

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

it somehow works now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

FabioLuporini
FabioLuporini approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

OK, thanks for the patience. This is now for me good to go.

@FabioLuporini FabioLuporini added arch jitting, archinfo, ... and removed compiler labels Jun 6, 2023
@FabioLuporini FabioLuporini changed the title compiler: Add ICX support arch: Add ICX support Jun 6, 2023
@FabioLuporini FabioLuporini merged commit 0678627 into master Jun 6, 2023
@FabioLuporini FabioLuporini deleted the add_icx_support branch June 6, 2023 05:43
@mloubout mloubout mentioned this pull request Jun 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabioLuporini FabioLuporini FabioLuporini approved these changes

@mloubout mloubout mloubout approved these changes

Assignees

@georgebisbas georgebisbas

Labels
arch jitting, archinfo, ...
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@georgebisbas @FabioLuporini @mloubout