Re: [fedora-arm] OpenMPI atomics patch

[Leif Lindholm]

Hi Jon,


First of all, thanks for doing this.


Now on to some picky feedback:

- The only current difference between the ARMV6 and the ARMV7 versions
is the use of cp15

  or DMB in the standalone barrier functions. Could this not be done
through some sort of

  macro instead?

- If we're playing around in here, might as well add support for (32-bit) ARMv8?

- Can you not set ompi_cv_asm__arch to the target file name rather
than copying around?


Together, this could end up with a config patchset something like
(freehanded, not tested):

---

        armv8*|armv7*)
            ompi_cv_asm_arch="ARM"
            OPAL_ASM_SUPPORT_64BIT=1
            OPAL_ASM_ARM_VERSION=7
            AC_DEFINE_UNQUOTED([OPAL_ASM_ARM_VERSION], [$OPAL_ASM_ARM_VERSION],
                               [What ARM assembly version to use])
            OMPI_GCC_INLINE_ASSIGN='"mov %0, #0" :
"=&r"(ret)'
            ;;

        armv6*)
            ompi_cv_asm_arch="ARM"
            OPAL_ASM_SUPPORT_64BIT=0
            OPAL_ASM_ARM_VERSION=6
            AC_DEFINE_UNQUOTED([OPAL_ASM_ARM_VERSION], [$OPAL_ASM_ARM_VERSION],
                               [What ARM assembly version to use])
            OMPI_GCC_INLINE_ASSIGN='"mov %0, #0" :
"=&r"(ret)'
            ;;

        armv5*linux*|armv4*linux*)
            # uses Linux kernel helpers for some atomic operations
            ompi_cv_asm_arch="ARMV5"
            OPAL_ASM_SUPPORT_64BIT=0
            OPAL_ASM_ARM_VERSION=5
            AC_DEFINE_UNQUOTED([OPAL_ASM_ARM_VERSION], [$OPAL_ASM_ARM_VERSION],
                               [What ARM assembly version to use])
            OMPI_GCC_INLINE_ASSIGN='"mov %0, #0" :
"=&r"(ret)'
            ;;

---

whilst containing one less source file, and not doing any copying as
part of the configure step.


I realise this would require at least touching generate-asm.pl, but it
might be possible

to get away with mostly gas macros and the end result would be a lot neater.


/

    Leif

OpenMPI atomics patch

[Jon Masters]

On 11/13/2012 02:27 PM, Leif Lindholm wrote:

>  First of all, thanks for doing this. 
No problem! Thanks for doing a bunch of the other OpenMPI bits :)

>  Now on to some picky feedback:
>  
>  - The only current difference between the ARMV6 and the ARMV7 versions is the use of
> cp15
>  
>    or DMB in the standalone barrier functions. Could this not be done through some sort
> of
>  
>    macro instead? 
That could be done. I had been planning to throw this over the wall at
the OMPI guys and say "hey, how do you actually want to fix this?"

>  - If we're playing around in here, might as well add support for
> (32-bit) ARMv8? 
Oh, 32-bit. Hmmm. I guess. I assume you mean A32, in which case we can
just reuse the ARMv7 code with a couple of extra lines?

>  - Can you not set ompi_cv_asm__arch to the target file name rather
> than copying around? 
Nope :) That's what I did at first (pretty much your stuff below).
Unfortunately, the OpenMPI codebase is a total mess. I also didn't want
to do assembly calls to kernel helpers, but it was easier to do that
than to figure out what the heck they are doing. However. If you want to
have another poke at that option (don't forget to search for all their
uses of the ompi_vc_asm arch) then I think that's awesome :)

Jon.

OpenMPI atomics patch

[Leif Lindholm]

On 13 November 2012 19:39, Jon Masters <jcm(a)redhat.com&gt; wrote:
>  > - The only current difference between the ARMV6 and the ARMV7
> versions
>  > is the use of cp15 or DMB in the standalone barrier functions. Could
>  > this not be done through some sort of macro instead?
>
>  That could be done. I had been planning to throw this over the wall at
>  the OMPI guys and say "hey, how do you actually want to fix this?" 
Yeah, problem is, they'll just come back to me :)
 
>  > - If we're playing around in here, might as well add support
> for
>  > (32-bit) ARMv8?
>
>  Oh, 32-bit. Hmmm. I guess. I assume you mean A32, in which case we can
>  just reuse the ARMv7 code with a couple of extra lines? 
Yup. Shouldn't require anything other than matching on it like it was v7.
 
>  > - Can you not set ompi_cv_asm__arch to the target file name
> rather
>  > than copying around?
>
>  Nope :) That's what I did at first (pretty much your stuff below).
>
>  Unfortunately, the OpenMPI codebase is a total mess. I also didn't want
>  to do assembly calls to kernel helpers, but it was easier to do that
>  than to figure out what the heck they are doing. However. If you want to
>  have another poke at that option (don't forget to search for all their
>  uses of the ompi_vc_asm arch) then I think that's awesome :) 
I had a look into this, and the required changes aren't actually _that_
invasive. You'd need changes to:
opal/asm/asm-data.txt (Adding ARMV5, but not sure it's strictly necessary)
opal/asm/generate-asm.pl (To handle ARMV5 in same special case as ARM)
opal/include/opal/sys/{architecture,atomic,timer}.h (appended at end)
and aforementioned barrier macros in ARM.asm

However, the armv4*linux support is not currently functional:
There was no cp15 in ARMv4, and there was no BLX until ARMv5TE.
You could conditionalize the cp15 and switch to
	mov lr, pc
	bx rd
sequences...

/
	Leif

diff -purN jcm/opal/include/opal/sys/architecture.h
leif/opal/include/opal/sys/architecture.h
--- jcm/opal/include/opal/sys/architecture.h    2012-04-03 15:29:45.000000000 +0100
+++ leif/opal/include/opal/sys/architecture.h   2012-11-14 12:03:12.517024691 +0000
@@ -36,7 +36,8 @@
 #define OMPI_SPARCV9_32     0061
 #define OMPI_SPARCV9_64     0062
 #define OMPI_MIPS           0070
-#define OMPI_ARM            0100
+#define OMPI_ARMV5          0100
+#define OMPI_ARM            0101
 
 /* Formats */
 #define OMPI_DEFAULT        1000  /* standard for given architecture */
diff -purN jcm/opal/include/opal/sys/atomic.h leif/opal/include/opal/sys/atomic.h
--- jcm/opal/include/opal/sys/atomic.h  2012-04-03 15:29:45.000000000 +0100
+++ leif/opal/include/opal/sys/atomic.h 2012-11-14 12:04:22.492089872 +0000
@@ -146,6 +146,8 @@ typedef struct opal_atomic_lock_t opal_a
 #include "opal/sys/alpha/atomic.h"
 #elif OPAL_ASSEMBLY_ARCH == OMPI_AMD64
 #include "opal/sys/amd64/atomic.h"
+#elif OPAL_ASSEMBLY_ARCH == OMPI_ARMV5
+#include "opal/sys/arm/atomic.h"
 #elif OPAL_ASSEMBLY_ARCH == OMPI_ARM
 #include "opal/sys/arm/atomic.h"
 #elif OPAL_ASSEMBLY_ARCH == OMPI_IA32
diff -purN jcm/opal/include/opal/sys/timer.h leif/opal/include/opal/sys/timer.h
--- jcm/opal/include/opal/sys/timer.h   2012-04-03 15:29:45.000000000 +0100
+++ leif/opal/include/opal/sys/timer.h  2012-11-14 12:03:53.585884563 +0000
@@ -79,6 +79,8 @@ BEGIN_C_DECLS
 /* don't include system-level gorp when generating doxygen files */ 
 #elif OPAL_ASSEMBLY_ARCH == OMPI_AMD64
 #include "opal/sys/amd64/timer.h"
+#elif OPAL_ASSEMBLY_ARCH == OMPI_ARMV5
+#include "opal/sys/arm/timer.h"
 #elif OPAL_ASSEMBLY_ARCH == OMPI_ARM
 #include "opal/sys/arm/timer.h"
 #elif OPAL_ASSEMBLY_ARCH == OMPI_IA32