Skip to content

Conversation

@bosilca
Copy link
Member

@bosilca bosilca commented Jan 11, 2021

Allow more flexibility on the support of AVX* extensions.

Use cached variables to prevent OMPI from checking specific architecture features. As an example this will allow to disable AVX512 while allowing AVX2.
Protect the code against mismatched flags between configure and make.
Document the list of of vectorial functions we need and the corresponding level of AVX, SSE required.

This is #8322 for 4.1

Fixes #8306

1. Consistent march flag order between configure and make.

2. op/avx: give the option to skip some tests

it is possible to skip some intrinsic tests by setting some environment variables to "no" before invoking configure:
 - ompi_cv_op_avx_check_avx512
 - ompi_cv_op_avx_check_avx2
 - ompi_cv_op_avx_check_avx
 - ompi_cv_op_avx_check_sse41
 - ompi_cv_op_avx_check_sse3

3. op/avx: update AVX512 flags

try
-mavx512f -mavx512bw -mavx512vl -mavx512dq
instead of
-march=skylake-avx512

since the former is less likely to conflict with user provided CFLAGS
(e.g. -march=...)

Thanks Bart Oldeman for pointing this.

4. op/avx: have the op/avx library depend on libmpi.so

Refs. open-mpi#8323

Signed-off-by: Gilles Gouaillardet <[email protected]>
Signed-off-by: George Bosilca <[email protected]>
1. Allow fallback to a lesser AVX support during make

Due to the fact that some distro restrict the compiule architecture
during make (while not setting any restrictions during configure) we
need to detect the target architecture also during make in order to
restrict the code we generate.

2. Add comments and better protect the arch specific code.

Identify all the vectorial functions used and clasify them according to
the neccesary hardware capabilities.
Use these requirements to protect the code for load and stores (the rest
of the code being automatically generated it is more difficult to
protect).

3. Correctly check for AVX* support.

Signed-off-by: George Bosilca <[email protected]>
The test now has the ability to add a shift to all or to any of the
input and output buffers to assess the impact of unaligned operations.

Signed-off-by: George Bosilca <[email protected]>
leofang added a commit to regro-cf-autotick-bot/openmpi-feedstock that referenced this pull request Jan 11, 2021
@jsquyres jsquyres merged commit 5e26699 into open-mpi:v4.1.x Jan 11, 2021
@leofang
Copy link

leofang commented Jan 11, 2021

Hi @bosilca Any chance this change would break --allow-run-as-root? On Conda-Forge we do mpiexec --allow-run-as-root ... as the CI runs in a docker, but after applying this patch I'm seeing this error:

mpiexec: Error: unknown option "--allow-run-as-root"

See https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=261970&view=logs&j=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e&t=841356e0-85bb-57d8-dbbc-852e683d1642&l=22345. All tests in conda-forge/openmpi-feedstock#71 currently fail due to this.

@jsquyres
Copy link
Member

No, this should have nothing to do with --allow-run-as-root.

I notice in your Azure run output:

+ /home/conda/feedstock_root/build_artifacts/openmpi-mpi_1610390538879/test_tmp/mpiexec.sh --help
$PREFIX/bin/mpiexec
mpiexec: Error: unknown option "--allow-run-as-root"

What's mpiexec.sh?

@leofang
Copy link

leofang commented Jan 11, 2021

Hi @jsquyres It's just a little script wrapping over mpiexec:
https://github.com/conda-forge/openmpi-feedstock/blob/master/recipe/mpiexec.sh
It seems we've been using it fine for quite a long time (before I knew anything about conda or mpi).

@leofang
Copy link

leofang commented Jan 11, 2021

@jsquyres
Copy link
Member

It looks like mpiexec.sh is just calling mpiexec (i.e., via relative path name, not absolute path name). Can you confirm that you're calling the "right" mpiexec?

@leofang
Copy link

leofang commented Jan 11, 2021

@jsquyres This is a clean/minimal environment so it's very likely impossible. We ensure conda's stuff is first found in $PATH. Tested locally on the same image:

$ docker run --rm -it quay.io/condaforge/linux-anvil-cuda:9.2
[conda@6fbb320a1381 ~]$ which mpiexec
/usr/bin/which: no mpiexec in (/opt/conda/bin:/opt/conda/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/home/conda/bin:/usr/local/cuda/bin)
[conda@6fbb320a1381 ~]$ command -v mpiexec
[conda@6fbb320a1381 ~]$ echo $?
1

That said, I'm happy to print the command out 🙂 Let me wait for the current run to finish...(I'm reverting this patch to see if the --allow-run-as-root error still happens or not).

@leofang
Copy link

leofang commented Jan 11, 2021

Let me wait for the current run to finish...(I'm reverting this patch to see if the --allow-run-as-root error still happens or not).

Hi @jsquyres Looks like a simple revert fixes the error: conda-forge/openmpi-feedstock@4dac571

Here's the CI output: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=262072&view=logs&j=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e&t=841356e0-85bb-57d8-dbbc-852e683d1642

Any thoughts for what else I can check?

@jsquyres
Copy link
Member

Wow, that's... weird. You've got me stumped on that, because this is what I get from a v4.1.x branch HEAD build:

$ mpirun --allow-run-as-root uptime
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:00 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
 14:12:01 up 492 days, 10:44, 82 users,  load average: 2.90, 2.91, 1.86
[2:12] savbu-usnic-a:~/g/ompi (v4.1.x)
$ mpirun --version
mpirun (Open MPI) 4.1.1a1

Report bugs to http://www.open-mpi.org/community/help/

The patch itself has zero to do with the mpirun command line parsing stuff. I don't know how applying that patch would disable the --allow-run-as-root CLI option. ☹️

@leofang
Copy link

leofang commented Jan 12, 2021

@jsquyres Question: which file(s) is this option implemented? Is it in one of the 4 files touched here?

@leofang
Copy link

leofang commented Jan 12, 2021

Apology, @jsquyres. I rerun our CI with the patch applied, and everything is green (so far). Looks like it was a transient error in Conda's build infrastructure that messed up with text patches among other things...😞 Thanks a lot (as always) for you help!

carlocab added a commit to carlocab/homebrew-core that referenced this pull request May 4, 2021
The `op-avx` flag disables building features that take advantage of AVX
instructions.

Since `open-mpi` is able to detect CPU features at runtime, disabling
this is not necessary, and simply leads to lower performance for users
with newer machines.

See also:

    open-mpi/ompi#8306
    open-mpi/ompi#8361
BrewTestBot pushed a commit to Homebrew/homebrew-core that referenced this pull request May 4, 2021
The `op-avx` flag disables building features that take advantage of AVX
instructions.

Since `open-mpi` is able to detect CPU features at runtime, disabling
this is not necessary, and simply leads to lower performance for users
with newer machines.

See also:

    open-mpi/ompi#8306
    open-mpi/ompi#8361

Closes #76619.

Signed-off-by: Sean Molenaar <[email protected]>
Signed-off-by: BrewTestBot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants