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

Fix ambiguous template special. errors with GCC 7+ #259

Closed
wants to merge 0 commits into from

Conversation

JM1
Copy link
Contributor

@JM1 JM1 commented Jul 29, 2018

Fix "ambiguous template specialization" errors like below with GCC 7 and later. #254

  include/El/blas_like/level1/Copy.hpp:562:27: error: ambiguous template specialization 'Copy<>' for 'void El::Copy(const El::SparseMatrix<El::Complex<double> >&
  , El::SparseMatrix<El::Complex<double> >&)'
     EL_EXTERN template void Copy \
                             ^~~~
  include/El/macros/Instantiate.h:54:27: note: in expansion of macro 'PROTO'
   # define PROTO_COMPLEX(T) PROTO(T)
                             ^~~~~
  include/El/macros/Instantiate.h:60:31: note: in expansion of macro 'PROTO_COMPLEX'
   # define PROTO_COMPLEX_DOUBLE PROTO_COMPLEX(Complex<double>)
                                 ^~~~~~~~~~~~~
  include/El/macros/Instantiate.h:120:1: note: in expansion of macro 'PROTO_COMPLEX_DOUBLE'
   PROTO_COMPLEX_DOUBLE
   ^~~~~~~~~~~~~~~~~~~~
  include/El/blas_like/level1/Copy.hpp:259:6: note: candidates are: 'template<class T> void El::Copy(const El::SparseMatrix<U>&, El::SparseMatrix<U>&)'
   void Copy( const SparseMatrix<T>& A, SparseMatrix<T>& B )
        ^~~~
  include/El/blas_like/level1/Copy.hpp:267:6: note:                 'template<class S, class T, class> void El::Copy(const El::SparseMatrix<U>&, El::SparseMatrix
  <V>&)'
   void Copy( const SparseMatrix<S>& A, SparseMatrix<T>& B )
        ^~~~

EDIT: Old commit can be found here.

Copy link
Member

@poulson poulson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jakob,

Thanks for putting this together!

Is there an explanation for the need for MPI_PROTO_COMPLEX(T) instead of MPI_PROTO(Complex<T>)?

@JM1
Copy link
Contributor Author

JM1 commented Jul 30, 2018

Take for example function Reduce (include/El/core/imports/mpi.hpp) with these two overloads:

template<typename Real, typename=EnableIf<IsPacked<Real>>>
void Reduce
( const Real* sbuf, Real* rbuf, int count, Op op, int root, Comm comm )
EL_NO_RELEASE_EXCEPT;

template<typename Real, typename=EnableIf<IsPacked<Real>>>
void Reduce
( const Complex<Real>* sbuf, Complex<Real>* rbuf, int count, Op op,
int root, Comm comm ) EL_NO_RELEASE_EXCEPT;

Macro MPI_PROTO(Complex<float>) (src/core/imports/mpi.cpp) instantiates Reduce with

template void Reduce
  ( const Complex<float>* sbuf, Complex<float>* rbuf, int count, Op op, int root, Comm comm )
  EL_NO_RELEASE_EXCEPT;

GCC 8 throws ambiguous template specialization errors because it cannot decide which candidate it should choose. Previous GCC revisions and Clang up to 6.0 (later revisions not tested) do compile this code as expected though.

To help GCC I explicitly instantiated those function templates in src/core/imports/mpi.cpp, e.g.

 template void Reduce<T> ...

But in order to instantiate the right Complex<Real> function overload the explicit template instantiations differ for some functions, e.g.

template void Reduce<T>
  ( const Complex<T>* sbuf, Complex<T>* rbuf, int count, Op op, int root, Comm comm )
  EL_NO_RELEASE_EXCEPT;

vs.

template void Reduce<Complex<T>>
  ( const Complex<T>* sbuf, Complex<T>* rbuf, int count, int root, Comm comm )
  EL_NO_RELEASE_EXCEPT;

Here it's Reduce<T> and there its Reduce<Complex<T>>. Therefore I had to clone and adapt the MPI_PROTO macro.

@poulson
Copy link
Member

poulson commented Jul 31, 2018

Thanks for the explanation! Though this sounds like a bug in GCC 8 (that is worth working around).

Is it not possible to use a chain of two macros to avoid hundreds of lines of redundancy between MPI_PROTO and MPI_PROTO_COMPLEX?

Also, the OpenMP include seems unrelated to this CL.

@JM1
Copy link
Contributor Author

JM1 commented Jul 31, 2018

A chain of macros would definitely be better. Any idea how this could look like?

I removed the OpenMP commit. Sorry, my fault.

@poulson
Copy link
Member

poulson commented Jul 31, 2018

Separating out the routines with equivalent definitions for both real and complex into a MPI_PROTO_BASE macro then calling it from MPI_PROTO and MPI_PROTO_COMPLEX would help. (And, at this point, renaming MPI_PROTO to MPI_PROTO_REAL makes sense).

@JM1
Copy link
Contributor Author

JM1 commented Jul 31, 2018

Of course! 🙈😅 I'm on it..

@JM1
Copy link
Contributor Author

JM1 commented Jul 31, 2018

Current draft is here.

@balay
Copy link

balay commented Aug 8, 2018

We are seeing this issue with petsc & v0.87.7

Would it be possilbe to have this fix on branch 0.87?

@poulson
Copy link
Member

poulson commented Aug 8, 2018

Thank you for the PR! The draft looks good and I think merging into 0.87 would be a great idea.

@balay
Copy link

balay commented Aug 9, 2018

BTW: this patch applies cleanly to 0.87 branch - and the build error goes away with it

[my test box is fedora 28 - with gcc version 8.1.1]

@JM1
Copy link
Contributor Author

JM1 commented Aug 9, 2018

I will open a PR for this draft commit rebased on master (build logs) once #263 is done.

@poulson
Copy link
Member

poulson commented Aug 9, 2018

FWIW, I just merged said commit despite it breaking the new build tests.

@benson31
Copy link
Contributor

benson31 commented Aug 9, 2018

I filed a bug report with GCC a while ago. It only seems to affect explicit template instantiation, not implicit instantiation. Last time I checked, GCC's development branch still won't accept this code, but that was last week.

mbakke pushed a commit to guix-mirror/guix that referenced this pull request Nov 19, 2023
The build of elemental is failed on GCC 7+:
elemental/Elemental#254

Fixed but not released yet in merge request:
elemental/Elemental#259

* gnu/packages/maths.scm (elemental): Update to 0.87.7-0.6eb15a0.

Change-Id: I333f1de160489035f1a8875173abdd57c03ef777
Signed-off-by: Eric Bavier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants