Getting Architecture Patch

Due to recent aarch64 and ppc64le port patches that have been sent, I have finished up this bit of code for CMake which detects the architecture by probing the compiler.

Carlo and C. Bergström can you two look at this patch and see if it adequately solves the detecting architecture problem for you both and add the proper macro into the GetArchitecture.cmake (this will make sense once you look at the patch)? I would like this to be committed before either port so both ports can use this new patch.

– Johnny

cmake_get_arch.patch (6.36 KB)

Hi Johnny

Thanks for this patch. I will give it a try on our machines and let you know.

Cheers

– Carlo

graycol.gif“Peyton, Jonathan L” —12/17/2014 01:18:14 PM—Due to recent aarch64 and ppc64le port patches that have been sent, I have finished up this bit of c

3 nits

  1. I think you can change it to #warn and avoid the compiler failure (please see what others are doing with similar approach and let me know). There’s also possibility of actually linking and doing a printf which would avoid the whole regex mess… Is there a strong reason for #error? (maybe faster?)

  2. Would anyone @Intel strongly oppose a global rename of 32e to something which is more commonly used (x86_64, amd64 /* joking */ or x8664)

  3. Can you include ARM64

Thanks

graycol.gif

New patch is attached.

  1. No and no. You can’t use #warn or #warning or the #pragma ways of doing a preprocessor warn/message/info because it isn’t standard (MSVC doesn’t have either #warn or #warning), #error is standard. Also, you can’t rely on running a small program, and EVERYTHING I’ve seen emphasizes this point. You can link but not actually run. There is a specific reason, cross-compiling. For example, if you relied on printf and are cross-compiling arm on an x86 host, then it won’t run on the x86 host system.

Since you asked what others do, I had looked at what CMake does specifically when detecting the compiler:

  • Creates and links a small executable that has a main() function in it with a few global variables defined like

char cmake_compiler_name = “COMPILER_NAME=[gcc]”;

char cmake_compiler_version_major = “COMPILER_VERISON_MAJOR=[00000004]”

char cmake_compiler_version_minor = “COMPILER_VERISON_MINOR=[00000008]”

etc.

These variables are set through an extensive search through compiler predefined macros and some clever preprocessing macros/tricks.

  • Searches through the executable via regex to find something like “COMPILER_NAME=[gcc]” and extracts gcc, then 4, then 8 etc.

  • An aside: because not every compiler produces a.out as default executable and not every compiler has –o flag to redirect output, it searches every file that was produced in the temporary work space besides the temporary.c file.

If you look at how LLVM does it, just look at GetHostTriple.cmake in llvm_root/cmake/modules/GetHostTriple.cmake. It calls config.guess on non-Windows platforms J.

  1. I personally don’t know about his one. 32e is embedded in lots of places including our internal tools and commercial product. I can check with everybody else of course and see what they think, but you would have to wait till after the holiday season for anything to happen about it.

  2. I did this in the new patch along with ppc64le

– Johnny

graycol.gif

cmake_get_arch_v2.patch (6.53 KB)

Ok looks good to me. If no objections from Carlo or others feel free to push.

In the other thread I sent an updated patch which would apply over top this change.

Thanks

graycol.gif

Hi Jonathan

I tried your patch on both p7 and p8 (le).

It works on p7, but I get the following error on p8:

CMake Error at cmake/HelperFunctions.cmake:37 (message):
check_variable(): arch = ppc64le is unknown

That string name (“ppc64le”) is the default for p8 machines and we would like it to be distinguished from the p7 one (“ppc64”) in iomp.

Can you please fix this? If you want I can do this, but please let me know if you wish so.

Thanks

– Carlo

graycol.gif“Peyton, Jonathan L” —12/17/2014 07:28:07 PM—New patch is attached. 1) No and no. You can’t use #warn or #warning or the #pragma ways of doing a

All

This patch adds up to Jonathan’s one with the ppc64le case. Be warned that it only implements auto-detection of the ppc64le architecture. I am working on integrating my own patch, which permits compilation of ppc64le, with this one.

(See attached file: cmake_get_arch_v3.patch)

Please, let me know of any issues

– Carlo

graycol.gif“Peyton, Jonathan L” —12/17/2014 07:28:07 PM—New patch is attached. 1) No and no. You can’t use #warn or #warning or the #pragma ways of doing a

cmake_get_arch_v3.patch (3.36 KB)

Hi

Attached a patch that includes both Jonathan’s own (cmake auto architecture detection) and some additional fix-ups by me to allow running on ppc64le architectures (Power 8).
My part of the patch follows suggestions made by Jim Cownie regarding the macro definition.
This also goes in the direction stated by Jim in one of his last e-mails related to a re-designing of architecture macros.

(See attached file: cmake_auto_arch_and_ppc64.patch)

Let me know if you prefer a patch as diffed from Jonathan’s one.

Thanks

– Carlo

graycol.gif“Peyton, Jonathan L” —12/17/2014 07:28:07 PM—New patch is attached. 1) No and no. You can’t use #warn or #warning or the #pragma ways of doing a

cmake_auto_arch_and_ppc64.patch (20.4 KB)

Carlo,

I tried your patch on both p7 and p8 (le).

It works on p7, but I get the following error on p8:

CMake Error at cmake/HelperFunctions.cmake:37 (message):
check_variable(): arch = ppc64le is unknown

That string name (“ppc64le”) is the default for p8 machines and we would like it to be distinguished from the p7 one (“ppc64”) in iomp.

Your patch has these lines in it:

-set(arch_possible_values 32e 32 arm ppc64)

+set(arch_possible_values 32e 32 arm ppc64 ppc64le)

So the problem above should be taken care of by this. The idea was to have my get_arch patch be independent of the recent architecture patches so that the commits can be in a logical order.

Ideally, the get_arch patch will be first, then the architecture ports are based off of that.

Some comments below on the ppc64le patch…

—Comment #1

Below, the KMP_ARCH_PPC macro should be removed, then undelete these two lines:

-# undef KMP_ARCH_PPC64

-# define KMP_ARCH_PPC64 1

This way, there is generic KMP_ARCH_PPC64 and then the more specific KMP_ARCH_PPC64BE and KMP_ARCH_PPC64LE macros.

I think Jim wanted to get rid of the KMP_ARCH_PPC anyways because it might confuse between 32-bit and 64-bit.

— a/runtime/src/kmp_os.h

+++ b/runtime/src/kmp_os.h

@@ -75,7 +75,10 @@

#define KMP_ARCH_X86 0

#define KMP_ARCH_X86_64 0

-#define KMP_ARCH_PPC64 0

+#define KMP_ARCH_PPC64_BE 0

+#define KMP_ARCH_PPC64_LE 0

graycol.gif

Hi Jonathan

Apologies for replying late. I believe this new version of the patch follows your guidelines (attached). I produced it starting from the last commits happened about a week ago or so.

C Bergström: I tested this patch on both ppc64, ppc64le, and x86_64. I do not have any available ARM 64 architecture - can you please try it and let me know if I broke anything?

Thanks

– Carlo

(See attached file: ppc64le-support.patch)

graycol.gif“Peyton, Jonathan L” —01/06/2015 04:03:30 PM—Carlo, “

ppc64le-support.patch (9.9 KB)

A whitespace change snuck in at here (extra tab)

+#define KMP_ARCH_X86_64 0

Otherwise the patch is probably in align with the intention

Hi

Did now see that one - I removed it in the attached version. Let me know if I broke something.

(See attached file: ppc64le-support.patch)

Thanks

– Carlo

graycol.gifC Bergström —01/15/2015 11:53:47 PM—A whitespace change snuck in at here (extra tab) +#define KMP_ARCH_X86_64 0

ppc64le-support.patch (9.9 KB)

Carlo,

I have applied your patch on top of the most recent changes (Intel® MIC changes), and also added a few changes:

diff --git a/runtime/CMakeLists.txt b/runtime/CMakeLists.txt

Got rid of this deletion of AARCH64

set(INTEL64 FALSE)

set(ARM FALSE)

-set(AARCH64 FALSE)

+set(PPC64BE FALSE)

+set(PPC64LE FALSE)

diff --git a/runtime/src/kmp_os.h b/runtime/src/kmp_os.h

Got rid of this deletion of AARCH64

-#define KMP_ARCH_PPC64 0

-#define KMP_ARCH_AARCH64 0

+#define KMP_ARCH_X86_64 0

In makefile.mk, I added a few places where I feel it should be checking for both ppc64 and ppc64le, you can see this in the attached patch file.

Check this out and see if it is correct.

To apply the patch on clean updated repository:

$ patch –p0 < ppc64le-support-v2.patch

– Johnny

graycol.gif

ppc64le-support-v2.patch (10.6 KB)

Hi Jonathan

Thanks for the changes (not sure why they did not show up in the conflict list after git merge).
This works for me on both ppc64 and ppc64le.

I noticed some minor changes to be done for ppc64le when using make:

  1. CACHE_LINE changes during compilation, from 128 (correct) to 64 (incorrect).

  2. The target directory is ppc64 instead of ppc64le.

For now I can live with both, if this is fine by you. I will submit a patch for this later.

Thanks again

– Carlo

graycol.gif“Peyton, Jonathan L” —01/16/2015 07:01:46 PM—Carlo, I have applied your patch on top of the most recent changes (Intel® MIC changes), and also ad

Hi

The ppc64le patch was conflicting with the latest push (small fixes on ARM64).
I have produced a new version of this patch that can be pushed against the current trunk (attached).

If everyone agrees on this, can someone please push this patch to the trunk?

(See attached file: ppc64le-support-v3.patch)

Thanks

– Carlo

graycol.gif“Peyton, Jonathan L” —01/16/2015 07:01:46 PM—Carlo, I have applied your patch on top of the most recent changes (Intel® MIC changes), and also ad

ppc64le-support-v3.patch (11 KB)

Carlo,

Thank you for raising those issues.

I believe I have fixed those two problems with this updated version of this patch. I could actually test the makefile logic by forcing a compile to ppc64le and seeing that it was attempting to put it in lin_ppc64le and also CACHE_LINE is 128 again for both ppc64 and ppc64le.

Andrey,

can you commit this?

To apply patch:

$ patch –p0 < ppc64le-support-v3.patch

– Johnny

graycol.gif

ppc64le-support-v3.patch (12.2 KB)

Committed, revision 226479.

The Carlo’s and Johnny’s patches differ only in order of lines (or elsif cases), or in comments. I have committed Johnny’s patch.

Thanks,

Andrey

graycol.gif