Hi Jeff,
To summarize the out-of-line assembler changes of this patch:
- The patch is functionally correct for ARMv7 (which we know, because the
code
doesn't change from the existing sources, it simply renames the file).
- It also appears to be functionally correct for ARMv6, given reports of
people testing it. The fact that the source is a direct copy of the ARMv7
file with the barrier operations substituted also suggests that it would
work. However, this duplication of functionally identical code seems
suboptimal to me.
- It *might* be functionally correct for ARMv5, although I have seen no
reports of it actually being tested on ARMv5 - only of being tested on
ARMv6/ARMv7 when successfully built for ARMv5.
- It is not functionally correct on ARMv4.
In addition to this, it effectively adds an additional build system layer,
by copying source files around at configuration time, on top of an already
very modular build system.
Now, the ARMv4/ARMv5 out-of-line code didn't exist at all before this, and
was only supported through the inline assembly, so this code would be useful
to keep, fix, and incorporate properly.
Basic point is - this is an insufficiently validated patch referred to as
"an ugly kludge" by the original author (Jon Masters@Red Hat), who created
it to be able to include it in the Fedora ARMv5 port. I has previously
provided suggestions for improvements, but it has still been submitted to
the Open MPI users list without any of those suggestions being acted on.
I admit to being slightly miffed with it being accepted and applied without
ever being mentioned on the Open MPI developers list - only on the users
list. A list to which I now find myself subscribed to without having asked
for or being told about - miffed again.
If the main purpose of accepting this patch is to provide a stopgap measure
for something better, I would much prefer simply incorporating your
CCASFLAGS
workaround into the configure script - removing the out-of-line asm
implementations of the atomics, but still providing a functional library for
the most common use-cases.
Something like:
Index: config/opal_config_asm.m4
===================================================================
--- config/opal_config_asm.m4 (revision 27881)
+++ config/opal_config_asm.m4 (working copy)
@@ -820,6 +820,7 @@
ompi_cv_asm_arch="ARM"
OPAL_ASM_SUPPORT_64BIT=0
OPAL_ASM_ARM_VERSION=6
+ CCASFLAGS+=" -march=armv7-a"
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)'
@@ -830,6 +831,7 @@
ompi_cv_asm_arch="ARM"
OPAL_ASM_SUPPORT_64BIT=0
OPAL_ASM_ARM_VERSION=5
+ CCASFLAGS+=" -march=armv7-a"
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)'
Then we can get on with rewriting this code properly with less urgency.
Regards,
Leif
> -----Original Message-----
> From: devel-boun...@open-mpi.org [mailto:devel-boun...@open-mpi.org] On
> Behalf Of Jeff Squyres (jsquyres)
> Sent: 22 January 2013 16:41
> To: Open MPI Developers
> Subject: Re: [OMPI devel] New ARM patch
>
> Leif --
>
> We talked about this a bit on our weekly call today.
>
> Just to be sure: are you saying that George's patches are *functionally
> correct* for ARM5/6/7 (and broken for ARM 4), but it would be better to
> organize the code a bit better?
>
> If that is correct, was ARM4 working before?
>
> If ARM4 was working before, how important is it? I.e., would it be ok
> to accept George's stuff for 1.7.0, and then accept any
> improvements/reshuffle/etc. from you for 1.7.1?
>
>
>
> On Jan 21, 2013, at 12:15 PM, Leif Lindholm <leif.lindh...@arm.com>
> wrote:
>
> > Hi George,
> >
> > Any chance of r27882 being reverted?
> >
> > As I told the Fedora guys when that patch originally surfaced[1],
> > I'm not overly fond of
> > - copying source files around as part of the configure step
> > - having separate source files for ARMv6 and ARMv7, when those
> differences
> > should be easily separated through macros (and would be reusable for
> 32-bit
> > ARMv8).
> >
> > Also, I might have mentioned that bit only on a separate thread on
> the Fedora list, but the ARMv4 support isn't actually correct (the ASM
> uses ARMv5-only operations).
> >
> > My alternate solution, the basic idea of which I posted over there
> [2] was to separate ARMv5 and earlier from ARM. Effectively separating
> the atomics implementation at the boundary where The ARM architecture
> got load-linked/store-conditional, rather than having a separate source
> file for every architecture version.
> >
> > [1] https://lists.fedoraproject.org/pipermail/arm/2012-
> November/004434.html
> > [2] https://lists.fedoraproject.org/pipermail/arm/2012-
> November/004460.html
> >
> > Best Regards,
> >
> > Leif
> >
> > -- IMPORTANT NOTICE: The contents of this email and any attachments
> are confidential and may also be privileged. If you are not the
> intended recipient, please notify the sender immediately and do not
> disclose the contents to any other person, use it for any purpose, or
> store or copy the information in any medium. Thank you.
> >
> >
> > _______________________________________________
> > devel mailing list
> > de...@open-mpi.org
> > http://www.open-mpi.org/mailman/listinfo.cgi/devel
>
>
> --
> Jeff Squyres
> jsquy...@cisco.com
> For corporate legal information go to:
> http://www.cisco.com/web/about/doing_business/legal/cri/
>
>
> _______________________________________________
> devel mailing list
> de...@open-mpi.org
> http://www.open-mpi.org/mailman/listinfo.cgi/devel
