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

debian mipsel build issues #148

Closed
aborrero opened this issue Jul 31, 2015 · 23 comments
Closed

debian mipsel build issues #148

aborrero opened this issue Jul 31, 2015 · 23 comments
Assignees

Comments

@aborrero
Copy link

Hi!

madness recently joined the Debian archive. However, it seems to fail to build from source in the mipsel arch [0].

The issue seems to be this:

[...]
../../../src/madness/world/timers.h:151:2: error: #error cpu_relax is not implemented!
 #error cpu_relax is not implemented!
  ^
[...]

Do you know how to address the issue?

best regards.

[0] https://buildd.debian.org/status/fetch.php?pkg=madness&arch=mipsel&ver=0.10-1&stamp=1437866412

@robertjharrison
Copy link
Contributor

Super cool ... how did we get into debian?

Anyway definitely worth making this work. Imagine

apt-get install madness

Scott ... could you please assist him?

@justusc
Copy link
Member

justusc commented Aug 1, 2015

The issue arises because the compiler does not define __x86_64__ or __i386. One of these two macros must be defined by the compiler so that the appropriate inline assembly is selected. In the case of cpu_relax(), the inline assembly is the same for x86_64 and i386. But for other parts of the MADNESS code, the assembly differs between the two.

Currently, cpu_relax only has implementation for x86_64, i386, BG/Q, and BG/P.

It may also be necessary to define various __SSE__, __SSE2__, etc. for SSE support, and/or __AVX__ or __AVX2__ for AVX support.

@aborrero
Copy link
Author

aborrero commented Aug 1, 2015

Could it be possible to get an implementation of cpu_relax() for mipsel? Could we simply use noop?

@jeffhammond
Copy link
Member

I think I can roll a portable implementation of all the stuff that currently requires assembly using a combination of C++11 and various Linux-isms. For example, cpu_relax can be mapped to nanosleep, for example.

Note that I do not have access to any MIPS platforms, but I can use my Parallela board at home to verify the generic build on ARM32.

Those that enjoy irony will appreciate that the Intel employee is doing this.

@jeffhammond jeffhammond self-assigned this Aug 1, 2015
@justusc
Copy link
Member

justusc commented Aug 2, 2015

As @jeffhammond said, we can probably implement a portable cpu_relax function, but other parts of the code may not be as easy. Specifically, the mtxm code in the tensor library. Others who know more may be able to answer that question better.

@jeffhammond
Copy link
Member

This should be fixed by #150, assuming that it passes CI.

Note that I fixed more than was requested. The cpu_relax() function is just the first thing you saw failing. The atomic class was only implemented for x86 and Blue Gene. I just wrote a C++11 implementation, which should be as portable as it gets.

@jeffhammond
Copy link
Member

@justusc We do not need to implement a portable mTxm because we already have a BLAS implementation of those functions that is used as a means of last resort. If we happen to have broken it somehow, it is easy to fix.

And yes, mTxm using BLAS is not the best implementation, but until we have users that regularly platforms besides x86 and Blue Gene, it should not be a priority. And MIPS is just about the lowest priority I can imagine - ARM and SPARC have a presence in HPC today, and we don't have any optimized support for those yet.

@justusc
Copy link
Member

justusc commented Aug 2, 2015

@jeffhammond thank you for the clarification.

@mbanck
Copy link
Contributor

mbanck commented Aug 2, 2015

@jeffhammond the Travis CI for #150 is displayed as failed, is that expected?

I'm happy to pull in your changeset for Debian to see how it works on the mips autobuilders, but if it's a WIP currently I'll hold off a bit more until it's merged.

@justusc
Copy link
Member

justusc commented Aug 2, 2015

@mbanck I checked the Travis CI output and it is definitely not working as intended.

@jeffhammond
Copy link
Member

Travis is not helping me figure out what's wrong. Can you help me
understand, Justus?

The cpu_relax fix is trivial so maybe we can pull that out of let the
Debian folks try just that.

@justusc
Copy link
Member

justusc commented Aug 2, 2015

The error is on line 47 of atomicint.h. The operator you intended to use was ">=".

@jeffhammond
Copy link
Member

I committed f679f8f to master to address the cpu_relax issue and nothing else. Can the Debian folks please try with that?

It's not clear to me that the asm-nop solution is portable, in which case I will do some investigating and find the right asm for every arch.

@jeffhammond
Copy link
Member

Thanks @justusc. I should have been more careful with that. I committed the fix in ac67249.

@mbanck
Copy link
Contributor

mbanck commented Aug 3, 2015

@jeffhammond I pulled in f679f8f and it looks much better now, i.e. it builds everywhere:: https://buildd.debian.org/status/package.php?p=madness

Will take a look at testsuite failures next when I get to it.

@jeffhammond
Copy link
Member

@mbanck Cool! Glad to hear it. But do you have use OpenMPI as the MPI dependency for MADNESS? I have much higher confidence in the correctness of MADNESS with MPI_THREAD_MULTIPLE if MPICH is used.

@mbanck
Copy link
Contributor

mbanck commented Sep 14, 2015

@jeffhammond right, I just noticed OpenMPI in Debian does not support MPI_THREAD_MULTIPLE when I tried to switch this on. I'll file an issue for the fact that it'd be nice if MADNESS figured out at configure time that it will crash on runtime if it is configured with --with-mpi-threads=multiple, but the MPI implementation does not provide that.

The main problem is that Open MPI is the default on most Debian architectures, but I will see about just using MPICH for MADNESS/MPQ3.

@jeffhammond
Copy link
Member

@mbanck I suppose we could detect OpenMPI as non-compliant with MPI_THREAD_MULTIPLE, but I'd prefer that users report this bug directly to them and shame them into supporting the MPI standard properly.

@mbanck
Copy link
Contributor

mbanck commented Sep 15, 2015

well, it's a matter of configuration - as I understand it the message has been mixed in the past (i.e., MPI_THREAD_MULTIPLE is either not safe, or lowers performance on single-thread runs). Plus Open MPI has been mostly unmaintained in the recent past in Debian. If you can point me to some (upstream?) documentation that clearly states whether MPI_THREAD_MULTIPLE is either ok or not ok on Open MPI right now, that would be very helpful

@jeffhammond
Copy link
Member

As long as open-mpi/ompi#157 is open, the answer is "not ok".

Why is Debian default to OpenMPI if it is unmaintained? Why don't you use
MPICH by default? I never had a single problem with MPICH on Debian.

@jeffhammond
Copy link
Member

@hjelmn tells me that Open-MPI 2.0 will support MPI_THREAD_MULTIPLE. I do not know what the release schedule for that version is, although there is a 2.x branch in their release repo.

@jeffhammond
Copy link
Member

@mbanck Please let me know if this issue has been resolved from a Debian perspective so that I can close the issue.

@mbanck
Copy link
Contributor

mbanck commented Dec 16, 2015

@jeffhammond I'd say this can be closed.

@evaleev evaleev closed this as completed Dec 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants