Bug 125 - Eigen3, MinGW: compilation failure, intrin.h does not exist
: Eigen3, MinGW: compilation failure, intrin.h does not exist
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - general
: 3.0
: x86 - 32-bit Windows
: --- normal
Assigned To: Eamon Nerbonne
:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-12-02 15:34 UTC by Matt
Modified: 2010-12-07 16:20 UTC (History)
4 users (show)



Attachments
Only do w64 intrin workaround in 64-bit mode (1.72 KB, patch)
2010-12-03 11:23 UTC, Eamon Nerbonne
no flags Details | Diff | Splinter Review
Intrinsics Mingw32 fix via extern "C" rather than intrin.h (2.28 KB, patch)
2010-12-03 23:30 UTC, Eamon Nerbonne
no flags Details | Diff | Splinter Review

Description Matt 2010-12-02 15:34:28 UTC
Currently trying out Eigen 3.0-beta2 (2010-10-15).

There seems to be a problem in file "eigen\Eigen\Core" -- right here:
// include files
#if (defined __GNUC__) && (defined __MINGW32__)
    #include <intrin.h>
    //including intrin.h works around a MINGW bug
http://sourceforge.net/tracker/?func=detail&atid=102435&aid=2962480&group_id=2435
    //in essence, intrin.h is included by windows.h and also declares
intrinsics (just as emmintrin.h etc. below do).  However,
    //intrin.h uses an extern "C" declaration, and g++ thus complains of
duplicate declarations with conflicting linkage.  The linkage for intrinsics
    //doesn't matter, but at that stage the compiler doesn't know; so, to avoid
compile errors when windows.h is included after Eigen/Core,
    //include intrin here.
#endif 

Despite the claim made in the above comment, there is no "intrin.h" file in
MinGW.

Tested both under the "g++-4.3.0-20080502-mingw32-alpha" as well as "g++ (GCC)
4.5.0" (which is the current official MinGW.org release, installed by the
automatic installer dated 2010-10-30, i.e. "mingw-get-inst-20101030.exe").

Incidentally, neither does "include/windows.h" in any of the above versions
include any header file containing "intrin" in its name.

This prevents a successful compilation -- e.g. if a file "example.cpp" contains

#include "Eigen/Dense"

in line 4, then the compilation fails:

In file included from ../Eigen/Dense:1:0,
                     from example.cpp:4:
../Eigen/Core:97:26: fatal error: intrin.h: No such file or directory
compilation terminated.

I've tried two workarounds:
(0) comment out #include <intrin.h> (since it doesn't exist on MinGW)
(1) since my CPU's ISA is x86, replace it with #include <x86intrin.h> (which
exists in MinGW since at least GCC 4.3.0)

Then, the code seemingly compiles, but it's unclear whether which one (if any)
correctly solves the problem.

On top of that, irrespectively of whether (0) or (1) was tried, experiencing
SEGFAULTs in SSE2/OpenMP environment -- please advise, should I file a separate
report on that or wait for the resolution of this one first (or should I
provide additional information on the second bug as well in this report, as
they may be related)?

Forum thread, with additional another problem (the above, SSE2) report:
http://forum.kde.org/viewtopic.php?f=74&t=91709
Comment 1 Benoit Jacob 2010-12-02 15:38:08 UTC
According to hg annotate, this code was introduced in changeset
6e860548ac904c87bb42207f9dfd24ac32fbfe2a by Eamon. He doesn't have a bugzilla
account yet, mailing him.
Comment 2 Benoit Jacob 2010-12-02 15:42:23 UTC
> On top of that, irrespectively of whether (0) or (1) was tried, experiencing
> SEGFAULTs in SSE2/OpenMP environment -- please advise, should I file a separate
> report on that or wait for the resolution of this one first (or should I
> provide additional information on the second bug as well in this report, as
> they may be related)?

This is almost certainly a separate bug, please file with steps to reproduce
(with testcase if applicable).
Comment 3 Eamon Nerbonne 2010-12-02 17:41:02 UTC
I'll take a look.

MingW was rather slow at getting a 4.5 release, so I've been using other builds
lately - but several (4.6 svn and 4.5 stable by equation.com, one by TDM, and
one by mingw64); and all of them have an intrin.h.

Could this be x86/x64 related perhaps?
Comment 4 Matt 2010-12-03 00:19:35 UTC
(In reply to comment #3)
> I'll take a look.
> 
> MingW was rather slow at getting a 4.5 release, so I've been using other builds
> lately - but several (4.6 svn and 4.5 stable by equation.com, one by TDM, and
> one by mingw64); and all of them have an intrin.h.
> 
> Could this be x86/x64 related perhaps?

Could be -- I've also tried compiling Eigen 3.0-beta2 on another computer, with
TDM MinGW, current 32-bit release (gcc-4.5.1-tdm-1) -- no "intrin.h" file
either (there exists "c:\MinGW32\lib\gcc\mingw32\4.5.1\include\x86intrin.h",
though; analogously to the official MinGW.org installation).

On a side note, I did try Equation.com's "gcc-4.5.1-32.exe" and "gcc-
4.6-20101127-32.exe" -- in contrast with MinGW, those third party packages have
"intrin.h" present; however, the compilation failed for other reasons (with a
long list of errors, not pretty) and I could not even find a workaround that
helped to move it forward. I suppose this is possibly low-priority though (at
least for me), as I do not plan to use third party packages (and I believe they
are less popular than official MinGW and TDM's MinGW). If supporting these is
important, I can file a separate bug report, too.

Still, getting even just the official MinGW to work would be great! :-)
Comment 5 Eamon Nerbonne 2010-12-03 02:05:01 UTC
Confirmed; and it indeed seems specific to the x86 version (which ships without
intrin.h).

Perhaps the workaround is unnecessary in mingw64 by now too; I'll check, and if
it's still necessary I'll just change the preprocessor guard to include a check
for x64, otherwise it can just be deleted.
Comment 6 Eamon Nerbonne 2010-12-03 02:05:19 UTC
Hmm, on second thought - this might not be x86 vs. x64 but mingw32 vs. mingw64
(
http://mingw-w64.sourceforge.net/
) - which obviously *can* run on x64, but also runs on x86.

Some more tests required...
Comment 7 Eamon Nerbonne 2010-12-03 11:23:55 UTC
Created attachment 45 [details] [review]
Only do w64 intrin workaround in 64-bit mode

Unfortunately, the mingw-w64 gcc builds still do require the intrin.h
workaround. I tried mingw-w64's 32-bit builds; these do have an intrin.h but
weirdly (fortunately!) don't require the intrin.h workaround.  In other words,
simply adding a check for 64-bit seems safe; that'll work on plain mingw, and
on mingw-w64, and on mingw-w64 for 32-bit systems.

The attached patch does the minor modification.
Comment 8 Benoit Jacob 2010-12-03 14:25:07 UTC
(In reply to comment #7)
> Created attachment 45 [details] [review]
> Only do w64 intrin workaround in 64-bit mode
> 
> Unfortunately, the mingw-w64 gcc builds still do require the intrin.h
> workaround. I tried mingw-w64's 32-bit builds; these do have an intrin.h but
> weirdly (fortunately!) don't require the intrin.h workaround.  In other words,
> simply adding a check for 64-bit seems safe; that'll work on plain mingw, and
> on mingw-w64, and on mingw-w64 for 32-bit systems.
> 
> The attached patch does the minor modification.

I didn't know this __SIZEOF_POINTER__, but if this works on mingw why not...

Can the people who reported this bug please test this patch?
Comment 9 Matt 2010-12-03 15:41:40 UTC
(In reply to comment #7)
> Created attachment 45 [details] [review]
> Only do w64 intrin workaround in 64-bit mode
> 
> Unfortunately, the mingw-w64 gcc builds still do require the intrin.h
> workaround. I tried mingw-w64's 32-bit builds; these do have an intrin.h but
> weirdly (fortunately!) don't require the intrin.h workaround.  In other words,
> simply adding a check for 64-bit seems safe; that'll work on plain mingw, and
> on mingw-w64, and on mingw-w64 for 32-bit systems.
> 
> The attached patch does the minor modification.

Eamon, thanks, will test.

By the way, have you considered using some of the flags mentioned here:
http://groups.google.com/group/mpir-devel/browse_thread/thread/e8c4aa0e4f4daafd

In particular: "MINGW64 defines [...] _WIN64 __MINGW64___".

If it works uniformly (across various distributions) well, then it might
express the intent more clearly. Unfortunately, I do not have access to a
x86-64 Windows OS here, so I won't be able to check the flags myself.
Comment 10 Benoit Jacob 2010-12-03 15:52:15 UTC
(In reply to comment #9)
> By the way, have you considered using some of the flags mentioned here:
> http://groups.google.com/group/mpir-devel/browse_thread/thread/e8c4aa0e4f4daafd
> 
> In particular: "MINGW64 defines [...] _WIN64 __MINGW64___".

I would prefer that. If that works.
Comment 11 Eamon Nerbonne 2010-12-03 23:25:10 UTC
(defined _WIN64) works too.

An alternative solution is perhaps cleaner:

The real problem here is that these intrinsics are defined as functions in
plain C or C++ even though they're actually compiler intrinsics.  So even
though it's nonsense, linkage matters; and in particular, intrin.h and
emmintrin.h include some of the same intrinsics but intrin.h does so in an
extern "C" section.

So, putting the SSE includes ???intrin.h in an extern "C" section in eigen also
resolves the problem, without requiring another header - I tested this; it
works.  In a sense, that's a cleaner solution all around.

So, we could just stick the ???intrin.h files eigen includes in an extern "C"
section.  That's likely to work in all variants of gcc (after all, linkage
doesn't matter), but to be conservative, we could do that for just mingw. 
Then, if a future version of mingw *does* include intrin.h (or any other
intrinsics header with extern "C" linkage, for that matter), Eigen doesn't
suddenly break.
Comment 12 Eamon Nerbonne 2010-12-03 23:30:02 UTC
Created attachment 48 [details] [review]
Intrinsics Mingw32 fix via extern "C" rather than intrin.h

This is a patch that sticks eigen's intrinsics header inclusions in extern "C"
for any mingw version (and does not mess with intrin.h at all).

Tested in the latest mingw.org build, the latest mingw-w64 x64 build, and the
latest mingw-w64 x86 build using the following:

#include <Eigen/Core>
#include <windows.h>
int main(int , char* []) {return 0;}
Comment 13 Matt 2010-12-04 00:31:12 UTC
(In reply to comment #7)
> Created attachment 45 [details] [review]
> Only do w64 intrin workaround in 64-bit mode
> 
> Unfortunately, the mingw-w64 gcc builds still do require the intrin.h
> workaround. I tried mingw-w64's 32-bit builds; these do have an intrin.h but
> weirdly (fortunately!) don't require the intrin.h workaround.  In other words,
> simply adding a check for 64-bit seems safe; that'll work on plain mingw, and
> on mingw-w64, and on mingw-w64 for 32-bit systems.
> 
> The attached patch does the minor modification.

OK, tested patch with __SIZEOF_POINTER__.

Compilation without errors, tested under:
- MinGW.org GCC "g++-4.3.0-20080502-mingw32-alpha"
- MinGW.org GCC "g++ (GCC) 4.5.0" 
- TDM MinGW GCC, current 32-bit release (gcc-4.5.1-tdm-1)
- MinGW-w64 (target: w32, x86) sezero's build
"mingw-w32-bin_i686-mingw_20101003_sezero.zip"

I guess this issue is done as far as I can test, thank you very much for the
help! :-)

// Regarding another bug -- still experiencing (OpenMP & SSE2+) bug,
interestingly it shows up under O0 and O2 optimizations while disappearing in
O1 and O3, will have to prepare a minimal-reproducible-case and test it under
various compilers, will report back (filing another report, as Benoit
suggested) when done.
Comment 14 Eamon Nerbonne 2010-12-07 12:22:51 UTC
Can a core contributor verify that one of the two patches is OK?  My personal
preference is for the second patch using extern "C"; that feels cleaner to me.
Comment 15 Benoit Jacob 2010-12-07 14:20:57 UTC
The extern "C" fix looks very much like the right fix, I love it. But why don't
we always do it? Why only for MinGW? I think I'm going to take your patch and
always do it, no #ifdef.
Comment 16 Eamon Nerbonne 2010-12-07 14:31:50 UTC
I didnt' always do it simply because I didn't test it on linux and whatnot - I
don't *think* it'll blow anything up, but I can't verify that. Removing the
#ifdef is definitely even cleaner :-).
Comment 17 Benoit Jacob 2010-12-07 16:15:10 UTC
Pushed. Thanks a lot. This extern "C" is something we theoretically needed
anyways since C headers are not guaranteed to be valid as C++ (C is not
strictly a subset of C++). Feel free to reopen if needed.
Comment 18 Benoit Jacob 2010-12-07 16:20:08 UTC
Sorry, I exaggerated a bit in my previous comment, extern "C" wont magically
make C code compile as C++, but you get the point :)

Note You need to log in before you can comment on or make changes to this bug.