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

Display human readable text for AArch64 vendors #13825

Merged
merged 14 commits into from Dec 17, 2019

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Nov 21, 2019

Refers to #13780

Also fixes a couple of typos in GCC flags.

@alalazo
Copy link
Member Author

alalazo commented Nov 21, 2019

Current output:

$ spack arch --known-targets 
Generic architectures (families)
    aarch64  arm  ppc  ppc64  ppc64le  ppcle  sparc  sparc64  x86  x86_64

GenuineIntel - x86
    i686  pentium2  pentium3  pentium4  prescott

GenuineIntel - x86_64
    nocona  core2  nehalem  westmere  sandybridge  ivybridge  haswell  broadwell  skylake  mic_knl  skylake_avx512  cannonlake  cascadelake  icelake

AuthenticAMD - x86_64
    k10  bulldozer  zen  piledriver  zen2  steamroller  excavator

IBM - ppc64
    power7  power8  power9

IBM - ppc64le
    power8le  power9le

Cavium - aarch64
    thunderx2

Fujitsu - aarch64
    a64fx

@tkameyama Can you give a look at this PR when you have some time?

@t-karatsu
Copy link
Contributor

t-karatsu commented Nov 22, 2019

Sorry for the delay in submitting the pull request due to some matters under consideration... Thank you @alalazo. And I will Correct the typo that was pointed out.

@alalazo
Copy link
Member Author

alalazo commented Nov 22, 2019

Sorry for the delay in submitting the pull request due to some matters under consideration...

No worries, I had some time so happy to help keep things in order. Thanks for the review!

Copy link
Contributor

@tkameyama tkameyama left a comment

Choose a reason for hiding this comment

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

Maybe the original source of implementer id is Arm Architecture Reference Manual Armv8, for Armv8-A architecture profile https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile D13.2.87 MIDR_EL1, Main ID Register (With version ea, page 3213)

@tldahlgren
Copy link
Contributor

tldahlgren commented Nov 22, 2019

Maybe the original source of implementer id is Arm Architecture Reference Manual Armv8, for Armv8-A architecture profile https://developer.arm.com/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile D13.2.87 MIDR_EL1, Main ID Register (With version ea, page 3213)

FWIW. The lists don't match though. That table has two entries not included here (i.e., 0xC0=Ampere, 0x4D=Motorola (or Freescale)) while the list here includes entries not in that table (i.e., 0x48=HiSilicon, 0x66=Faraday).

Another partial list can be found for gcc at https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/aarch64-cores.def, which includes: 0x68=HXT (not here) and 0x48=HiSilicon (here).

The partial list at https://patchwork.kernel.org/patch/10524949/ indicates 0x61=Apple (not here).

@alalazo
Copy link
Member Author

alalazo commented Nov 26, 2019

@tkameyama @t-karatsu Can you please have a look at the latest updates to clang in 31f6286 and 9ce0b6b and double check the features enabled?

@alalazo
Copy link
Member Author

alalazo commented Nov 26, 2019

This is ready for a further review.

Copy link
Contributor

@tldahlgren tldahlgren left a comment

Choose a reason for hiding this comment

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

Thanks for adding the extra vendor ids.

This LGTM though I have not double-checked any of the flags under the assumption you've been getting that feedback from others.

Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

@alalazo: LGTM except for a minor request -- I think all the mappings should go in the JSON, so that someone can easily make another set of bindings for the archspec library. Basically, if it's knowledge gleaned from somewhere, we should consolidate it in one place: microarchitectures.json

lib/spack/llnl/util/cpu/detect.py Outdated Show resolved Hide resolved
lib/spack/llnl/util/cpu/detect.py Outdated Show resolved Hide resolved
Comment on lines 1210 to 1215
{
"versions": "9:",
"family": "aarch64",
"flags": "-march={family} -mcpu=thunderx2t99"
}
]
Copy link
Contributor

@t-karatsu t-karatsu Nov 27, 2019

Choose a reason for hiding this comment

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

Thanks @alalazo, but these options raise errors following:

[ogura@langtx202 bin]$ ./clang test.c -march=aarch64 -mattr=+v8.1a,+fp-armv8,+neon,+crc,+crypto
clang-8: error: unknown argument: '-mattr=+v8.1a,+fp-armv8,+neon,+crc,+crypto'
clang-8: error: the clang compiler does not support '-march=aarch64'

So, options for llc and clang seem different. I considered options for clang compiler commands. Currently I will Check whether it works normally.

diff --git a/lib/spack/llnl/util/cpu/microarchitectures.json b/lib/spack/llnl/util/cpu/microarchitectures.json
index 3b58899..e37327a 100644
--- a/lib/spack/llnl/util/cpu/microarchitectures.json
+++ b/lib/spack/llnl/util/cpu/microarchitectures.json
@@ -1196,10 +1196,16 @@
             "flags": "-mcpu=thunderx2t99"
           }
         ],
-        "clang": {
-          "versions": ":",
-          "flags": "-march=armv8-a -mcpu=generic"
-        }
+        "clang": [
+          {
+            "versions": "3.9:",
+            "flags": "-march=armv8.1-a+crc+crypto"
+          },
+          {
+            "versions": "5.0:",
+            "flags": "-mcpu=thunderx2t99"
+          }
+        ]
       }
     },
     "a64fx": {
@@ -1246,10 +1252,16 @@
             "flags": "-arch=armv8.2a+crc+aes+sh2+fp16+sve -msve-vector-bits=512"
           }
         ],
-        "clang": {
-          "versions": ":",
-          "flags": "-march=armv8-a -mcpu=generic"
-        }
+        "clang": [
+          {
+            "versions": "3.9:",
+            "flags": "-march=armv8.2-a+crc+crypto+fp16"
+          },
+          {
+            "versions": "5.0:",
+            "flags": "-march=armv8.2-a+crc+crypto+fp16+sve"
+          }
+        ]
       }
     },
     "arm": {

I am sorry if there is a lack of consideration here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @t-karatsu. You're completely correct. I think we need to revisit also the flags that are passed to x86-64. I tried to disassemble some vectorizable code and it seems clang does not give the same meaning to flags that are also used by its assembler. I'll try to see if I can find some reference for it and eventually post here what I'll find. @tgamblin FYI

Copy link
Member Author

Choose a reason for hiding this comment

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

For reference, when I researched this I couldn't find a good source of information. Even clang manuals suggest options that are either not recognized on command line or don't work as described. The best source of information I could find at the time was this SO answer.

Copy link
Member Author

Choose a reason for hiding this comment

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

@becker33 I think you might also be interested in the discussion above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Further update: I found this discussion on how clang opt and llc relate to each other (and goes on a 4 stage manual build to use them). There's also this other question that looks exactly what we need, but has no answer.

I'll continue looking, in the meanwhile I posted a message to cfe users - let's see if something comes out from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, when I researched this I couldn't find a good source of information.

How about clang preprocessor test code?
https://github.com/llvm/llvm-project/blob/master/clang/test/Preprocessor/aarch64-target-features.c

Copy link
Member Author

Choose a reason for hiding this comment

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

@tkameyama @t-karatsu I got access today to a thunderx2 machine. Will try out the flags there.

How about clang preprocessor test code?

That should work thanks. The front-end driver seems consistent with GCC in terms of flags which is also what is briefly written here. Will update this PR asap and ping you when done.

Co-Authored-By: t-karatsu <49965247+t-karatsu@users.noreply.github.com>
@alalazo
Copy link
Member Author

alalazo commented Nov 29, 2019

@t-karatsu @tkameyama @tgamblin PR ready for another review

@t-karatsu
Copy link
Contributor

t-karatsu commented Dec 2, 2019

Thank you grateful @alalazo ! I operated in spack with this code, it worked very good.

[tx202 spack]$ spack arch
linux-centos7-thunderx2
[tx202 spack]$  spack arch --known-targets
Generic architectures (families)
    aarch64  arm  ppc  ppc64  ppc64le  ppcle  sparc  sparc64  x86  x86_64
...
Fujitsu - aarch64
    a64fx
...
Cavium - aarch64
    thunderx2

env in installed OSS:
- gcc@4.8.5
    SPACK_TARGET_ARGS=-march=armv8-a; export SPACK_TARGET_ARGS
- gcc@9.1.0
    SPACK_TARGET_ARGS=-mcpu=thunderx2t99; export SPACK_TARGET_ARGS

But please tell me about one code.

@tgamblin tgamblin added this to Todo in Spack v0.14.0 release via automation Dec 3, 2019
@alalazo
Copy link
Member Author

alalazo commented Dec 11, 2019

@tgamblin ping

Spack v0.14.0 release automation moved this from Todo to In review Dec 16, 2019
@tgamblin tgamblin merged commit 5eca4f1 into spack:develop Dec 17, 2019
Spack v0.14.0 release automation moved this from In review to Done Dec 17, 2019
@alalazo alalazo deleted the fixes/vendor_on_arm branch December 17, 2019 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants