Fix ARM issues #1515

georgebisbas · 2020-11-20T14:39:37Z

Supersedes 1510
Fix ARM issue, improve platform autodetection.

autodetect Features (flags in older versions) necessary for vectorization
assert lscpu and psutil where possible
Solves issues when splitting : due to https://askubuntu.com/questions/1248273/lscpu-vulnerabilities

TODO:
Improve par-nested override for a global solution

Tested on Isambard's Marvell ThunderX2

codecov · 2020-11-20T14:48:24Z

Codecov Report

Merging #1515 (3eefea2) into master (78c8a6f) will decrease coverage by 0.67%.
The diff coverage is 62.62%.

@@            Coverage Diff             @@
##           master    #1515      +/-   ##
==========================================
- Coverage   87.17%   86.50%   -0.68%     
==========================================
  Files         203      206       +3     
  Lines       28950    29009      +59     
  Branches     3910     3914       +4     
==========================================
- Hits        25238    25094     -144     
- Misses       3274     3469     +195     
- Partials      438      446       +8

Impacted Files	Coverage Δ
devito/core/cpu.py	`98.92% <ø> (-0.17%)`	⬇️
devito/core/arm.py	`30.95% <30.95%> (ø)`
devito/archinfo.py	`51.77% <50.00%> (-0.56%)`	⬇️
devito/core/__init__.py	`100.00% <100.00%> (ø)`
devito/core/intel.py	`100.00% <100.00%> (ø)`
devito/core/power.py	`100.00% <100.00%> (ø)`
tests/test_gpu_openacc.py	`37.34% <0.00%> (-62.66%)`	⬇️
tests/test_gpu_openmp.py	`73.48% <0.00%> (-25.97%)`	⬇️
devito/core/gpu_openacc.py	`45.06% <0.00%> (-25.93%)`	⬇️
devito/compiler.py	`48.56% <0.00%> (-6.71%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c8a6f...3eefea2. Read the comment docs.

FabioLuporini · 2020-11-22T18:35:46Z

should the title be updated?

devito/archinfo.py

devito/core/cpu.py

FabioLuporini · 2020-11-24T13:28:28Z

devito/archinfo.py

-                flags = [i for i in lines if i.startswith('flags')][0]
+                # ARM Thunder X2 is using 'Features' instead of 'flags'
+                flags = [i for i in lines if (i.startswith('Features')
+                         or i.startswith('flags'))][0]


nitpicking: the or i.startswith('flags)' should be aligned with i.startswith('Featuers')

But then flake8 complains (?)

it will not

devito/archinfo.py

FabioLuporini · 2020-11-24T13:34:27Z

devito/archinfo.py

@@ -64,12 +66,16 @@ def get_cpu_brand():

        cpu_info['flags'] = get_cpu_flags()
        cpu_info['brand'] = get_cpu_brand()
+        if cpu_info['brand'] is None:
+            cpu_info['brand'] = cpuinfo.get_cpu_info().get('raw_arch_string')


Actually, this should be moved here in place of the return None right?

As a new try-except pair I would say (?)

devito/archinfo.py

FabioLuporini · 2020-11-27T12:57:55Z

devito/archinfo.py

-                flags = [i for i in lines if i.startswith('flags')][0]
+                # ARM Thunder X2 is using 'Features' instead of 'flags'
+                flags = [i for i in lines if (i.startswith('Features')
+                         or i.startswith('flags'))][0]


it will not

devito/archinfo.py

FabioLuporini · 2020-11-27T13:01:24Z

devito/archinfo.py

-        cpu_info['flags'] = ci.get('flags')
-        cpu_info['brand'] = ci.get('brand')
+    try:
+        if 'flags' not in cpu_info:


is it true that the only possibility for flags not to be in cpu_info is that lines is empty? because if we enter the if lines: at line 34, then we go through lines 70-71, hence cpu_info['flags'] should be defined?

This is the part where I do not know what happens in OSX, tests fail only there.

FabioLuporini · 2020-11-27T13:03:18Z

devito/archinfo.py

+        if 'flags' not in cpu_info:
+            # Fallback
+            ci = cpuinfo.get_cpu_info()
+            cpu_info['flags'] = ci.get('flags')


so going back to my original comment. What I was actually suggesting was to move the content of the try-except in place of current line 48 -- instead of returning None, we return the cpuinfo.get_cpu_info().get('flags') ?

again, I did that the previous days and was failing on OSX

FabioLuporini · 2020-11-27T13:04:30Z

devito/archinfo.py


    # Detect number of logical cores
+    try:
+        if cpu_info['brand'] == 'aarch64':
+            # In some ARM processors psutils and lscpu fail to detect cores correctly


this comment doesn't make much sense does it? you say that lscpu fails, but then you actually use it below?

omg nice catch, I do not remember what I was thinking at that time.

FabioLuporini · 2020-11-27T13:05:33Z

devito/archinfo.py

+            # In some ARM processors psutils and lscpu fail to detect cores correctly
+            logical = psutil.cpu_count(logical=True)
+            physical = psutil.cpu_count(logical=False)
+            if physical != (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)']):


if not A: B = A

=>

B = A

True. This is a leftover from using it to print a warning message thaty was later removed.
warning...core autodetection is not reliable....using lscpu....

devito/archinfo.py

FabioLuporini · 2020-12-01T08:50:02Z

devito/archinfo.py

            try:
-                physical = lscpu()['Core(s) per socket'] * lscpu()['Socket(s)']
+                physical = (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)'])


why adding parentheses?

FabioLuporini · 2020-12-01T08:51:15Z

devito/archinfo.py

@@ -107,15 +119,16 @@ def get_cpu_brand():
        # Fallback 1: it should now be fine to use psutil
        physical = psutil.cpu_count(logical=False)
        if not physical:
-            # Fallback 2: we might end up here on more exotic platforms such a Power8
-            # Hopefully we can rely on `lscpu`
+            # Fallback 2: we might end up here


can we avoid breaking this line into three?

also, typo: 'such a' -> 'such as'

should we now drop the "or due to erroneous autodetection" part?

devito/archinfo.py

FabioLuporini · 2020-12-01T08:52:21Z

devito/archinfo.py

@@ -258,7 +271,8 @@ def lscpu():
    if output:
        lines = output.decode("utf-8").strip().split('\n')
        mapper = {}
-        for k, v in [tuple(i.split(':')) for i in lines]:
+        # Using split(':', 1) to avoid splitting lines with security issues


"with security issues" . Can you be more precise? Nobody will understand it

# Using split(..... # Example: ...

What about:

# Using split(':', 1) to avoid splitting lines where lscpu shows vulnerabilities # on some CPUs: https://askubuntu.com/questions/1248273/lscpu-vulnerabilities

FabioLuporini · 2020-12-01T08:53:48Z

devito/archinfo.py

                return flags.split(':')[1].strip().split()
            except:
-                return None
+                pass


why this change? this will return None anyway, so it's clearer the other way

Why will this return None?

I think I get your point. TO my understanding if the last pair of try-except in each function
get_cpu_xxx
is returning None, then we are fine.

devito/archinfo.py

FabioLuporini · 2020-12-02T07:57:57Z

devito/archinfo.py

-        cpu_info['flags'] = ci.get('flags')
-        cpu_info['brand'] = ci.get('brand')
+    try:
+        if cpu_info['flags'] is None:


I don't see the point for this try-except. The following should suffice?

if not cpu_info.get('flags'): cpu_info['flags'] = cpuinfo.get_cpu_info().get('flags')

same for the other try-except below

FabioLuporini · 2020-12-02T07:59:25Z

devito/archinfo.py

+    except KeyError:
+        cpu_info['brand'] = cpuinfo.get_cpu_info().get('brand')
+
+    # Detect number of physical and logical cores


Drop this comment and replace with the comment below "Special case: in ... " (note that "In" -> "in" . Colons don't want capital letters afterwards)

FabioLuporini · 2020-12-02T08:01:18Z

devito/archinfo.py

+            # correctly so we use lscpu()
+            logical = psutil.cpu_count(logical=True)
+            physical = (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)'])
+            cpu_info['logical'] = logical


I still think that the issue with ARM is the correct detection of number of physical cores, not the logical cores. So I would:

drop the logical detection here

move this try-except after the "logical core detection" part, ie below line 110

Yes, but getting the logical here helps as return cpu_info and avoid all the other lines after 110

I was suggesting to move it right between lines 103 and 104, so you would still avoid "those other lines"

if we do so, I think, it'd become a bit neater

FabioLuporini · 2020-12-03T21:51:50Z

devito/archinfo.py

+            # correctly so we use lscpu()
+            logical = psutil.cpu_count(logical=True)
+            physical = (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)'])
+            cpu_info['logical'] = logical


I was suggesting to move it right between lines 103 and 104, so you would still avoid "those other lines"

FabioLuporini · 2020-12-03T21:52:20Z

devito/archinfo.py

+            # correctly so we use lscpu()
+            logical = psutil.cpu_count(logical=True)
+            physical = (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)'])
+            cpu_info['logical'] = logical


if we do so, I think, it'd become a bit neater

devito/archinfo.py

mloubout · 2020-12-04T12:50:41Z

Considering the amount of try/except and if/else, would it be good to separate archinfo in arch specific case and just check them one by one like

for arch in [intel, arm, ....]
     if found(arch):
         do what's needed
         break

FabioLuporini · 2020-12-04T13:18:52Z

devito/archinfo.py

+    try:
+        if 'arm' in cpu_info['brand']:
+            physical = (lscpu()['Core(s) per socket'] * lscpu()['Socket(s)'])
+            cpu_info['physical'] = physical


can this be a single line or does it not fit the 90 chars limit?

FabioLuporini

I actually think I'm quite happy with the current state of this PR

FabioLuporini · 2020-12-07T16:25:11Z

devito/core/__init__.py

+                             CustomOperator)
+from devito.core.intel import (Intel64Operator, Intel64OpenMPOperator,
+                               Intel64FSGOperator, Intel64FSGOpenMPOperator)
+from devito.core.arm import (ArmOperator, ArmOpenMPOperator)


no need for parentheses

FabioLuporini · 2020-12-07T16:25:16Z

devito/core/__init__.py

+from devito.core.intel import (Intel64Operator, Intel64OpenMPOperator,
+                               Intel64FSGOperator, Intel64FSGOpenMPOperator)
+from devito.core.arm import (ArmOperator, ArmOpenMPOperator)
+from devito.core.power import (PowerOperator, PowerOpenMPOperator)


FabioLuporini · 2020-12-07T16:25:31Z

devito/core/arm.py

+__all__ = ['ArmOperator', 'ArmOpenMPOperator']
+
+
+ArmOperator = CPU64Operator


is this really needed?

HHmm... not needed but was it needed before?

FabioLuporini · 2020-12-07T16:25:35Z

devito/core/arm.py

+
+
+ArmOperator = CPU64Operator
+ArmOpenMPOperator = CPU64OpenMPOperator


FabioLuporini · 2020-12-08T16:06:59Z

devito/core/intel.py

+    _specialize_iet = CPU64OpenMPOperator._specialize_iet
+
+
+PowerOperator = CPU64Operator


what is this?

FabioLuporini · 2020-12-08T16:07:09Z

devito/core/intel.py

+PowerOperator = CPU64Operator
+PowerOpenMPOperator = CPU64OpenMPOperator
+
+ArmOperator = CPU64Operator


devito/core/intel.py

FabioLuporini · 2020-12-11T14:51:58Z

devito/core/arm.py

+                               optimize_halospots, hoist_prodders, relax_incr_dimensions)
+from devito.tools import timed_pass
+
+


one extra blank line?

FabioLuporini

OK now, thanks

FabioLuporini

OK, good now, thanks. Merging

FabioLuporini · 2020-12-15T08:15:19Z

Merged.

georgebisbas requested review from FabioLuporini and mloubout November 20, 2020 19:00

georgebisbas changed the title ~~[NOT FOR MERGE][TESTING ONLY] Fix arm issues~~ Fix arm issues Nov 22, 2020

georgebisbas force-pushed the fix-arm-issues-bisbas branch 2 times, most recently from c2894cf to 29967fc Compare November 22, 2020 21:45

georgebisbas mentioned this pull request Nov 22, 2020

arch: fix archinfo and misc for ARM #1510

Closed

mloubout added compiler enhancement labels Nov 22, 2020

FabioLuporini requested changes Nov 23, 2020

View reviewed changes

devito/archinfo.py Outdated Show resolved Hide resolved

devito/archinfo.py Outdated Show resolved Hide resolved

devito/core/cpu.py Outdated Show resolved Hide resolved

georgebisbas force-pushed the fix-arm-issues-bisbas branch 3 times, most recently from 0b7e78d to b106ca2 Compare November 23, 2020 17:30

mloubout approved these changes Nov 24, 2020

View reviewed changes

georgebisbas requested a review from FabioLuporini November 24, 2020 11:45

FabioLuporini requested changes Nov 24, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch from 805fa2a to 9cf091a Compare November 26, 2020 08:19

georgebisbas commented Nov 26, 2020

View reviewed changes

devito/archinfo.py Outdated Show resolved Hide resolved

georgebisbas force-pushed the fix-arm-issues-bisbas branch from eb6eac8 to a2cf2b3 Compare November 26, 2020 11:08

georgebisbas mentioned this pull request Nov 26, 2020

Testing issues on ARM #1500

Closed

georgebisbas requested a review from FabioLuporini November 27, 2020 07:29

georgebisbas changed the title ~~Fix arm issues~~ Fix ARM issues Nov 27, 2020

FabioLuporini reviewed Nov 27, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch from 9112899 to 8638a12 Compare November 30, 2020 18:29

georgebisbas requested a review from FabioLuporini December 1, 2020 08:48

FabioLuporini reviewed Dec 1, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch from c3f3ddd to bd1202c Compare December 1, 2020 08:55

georgebisbas linked an issue Dec 1, 2020 that may be closed by this pull request

Platform autodetection fails on ARM #1043

Closed

georgebisbas force-pushed the fix-arm-issues-bisbas branch from 0040b64 to 0055eea Compare December 1, 2020 21:33

FabioLuporini requested changes Dec 2, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch from ea61b62 to b339bbf Compare December 2, 2020 10:40

FabioLuporini reviewed Dec 3, 2020

View reviewed changes

FabioLuporini reviewed Dec 4, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch 2 times, most recently from 9bceb52 to 3b1ab1a Compare December 4, 2020 13:37

georgebisbas requested a review from FabioLuporini December 7, 2020 13:54

FabioLuporini requested changes Dec 8, 2020

View reviewed changes

jaimesouza added 2 commits December 11, 2020 14:42

arch: fix archinfo and misc for ARM

41b104f

cpu: override _specialize_iet to drop avoid_denormals on ArmOperator

145422d

georgebisbas force-pushed the fix-arm-issues-bisbas branch from 0ff18e1 to 9bd2e95 Compare December 11, 2020 14:42

FabioLuporini reviewed Dec 11, 2020

View reviewed changes

georgebisbas force-pushed the fix-arm-issues-bisbas branch from df0acb2 to f730bc5 Compare December 11, 2020 15:04

georgebisbas added 4 commits December 11, 2020 17:05

arch: improve platform autodetection

9e12f26

cpu: split operator families

6cc8503

arch: drop redundant code

f67f8fd

arch: minor

2da5b8e

georgebisbas force-pushed the fix-arm-issues-bisbas branch from f730bc5 to 2da5b8e Compare December 11, 2020 15:06

FabioLuporini approved these changes Dec 14, 2020

View reviewed changes

Merge branch 'master' into fix-arm-issues-bisbas

3eefea2

FabioLuporini approved these changes Dec 15, 2020

View reviewed changes

FabioLuporini merged commit a18d10a into master Dec 15, 2020

FabioLuporini deleted the fix-arm-issues-bisbas branch December 15, 2020 08:15

		__all__ = ['ArmOperator', 'ArmOpenMPOperator']


		ArmOperator = CPU64Operator



		ArmOperator = CPU64Operator
		ArmOpenMPOperator = CPU64OpenMPOperator

		_specialize_iet = CPU64OpenMPOperator._specialize_iet


		PowerOperator = CPU64Operator

		optimize_halospots, hoist_prodders, relax_incr_dimensions)
		from devito.tools import timed_pass

Fix ARM issues #1515

Fix ARM issues #1515

Conversation

georgebisbas commented Nov 20, 2020 • edited Loading

codecov bot commented Nov 20, 2020 • edited Loading

Codecov Report

FabioLuporini commented Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout commented Dec 4, 2020

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

FabioLuporini left a comment

Choose a reason for hiding this comment

FabioLuporini commented Dec 15, 2020

georgebisbas commented Nov 20, 2020 •

edited

Loading

codecov bot commented Nov 20, 2020 •

edited

Loading

FabioLuporini commented Nov 22, 2020 •

edited

Loading