rjhogan / Adept-2

Combined array and automatic differentiation library in C++
http://www.met.reading.ac.uk/clouds/adept/
Apache License 2.0
164 stars 29 forks source link

_mm_undefined_ps missing on old clang #7

Closed djsutherland closed 6 years ago

djsutherland commented 6 years ago

Packet.h has test code to define _mm_undefined_ps if it's missing on old GCC. Old Clangs, though, also don't have this instruction, causing build failures: example Travis failure.

I was able to work around it in the conda-forge package we're adding with a dumb patch, which I'm only applying for our OSX builds. You shouldn't put this in Adept, because it's incorrect (if _mm_undefined_ps is defined as a function rather than a macro, as it actually is, it'll override that definiton), but there should probably be a version check for clang too.

rjhogan commented 6 years ago

P {margin-top:0;margin-bottom:0;}

Thanks - yes I found this problem recently with clang and was planning to post a fix soon. Regards, Robin.

From: Dougal J. Sutherland [notifications@github.com] Sent: 30 January 2018 18:54 To: rjhogan/Adept-2 Cc: Subscribed Subject: [rjhogan/Adept-2] _mm_undefined_ps missing on old clang (#7)

Packet.h has test code to define _mm_undefined_ps if it's missing on old GCC. Old Clangs, though, also don't have this instruction, causing build failures:

example Travis failure. I was able to work around it in the conda-forge package we're adding with a dumb patch, which I'm only applying for our OSX builds. You shouldn't put this in Adept, because it's incorrect (if _mm_undefined_ps is defined as a function rather than a macro, as it actually is, it'll override that definiton), but there should probably be a version check for clang too. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

yurivict commented 6 years ago

Could you please fix this? I am seeing the same problem in the FreeBSD port in systems with older clang versions.

rjhogan commented 6 years ago

Should be fixed in latest push.

yurivict commented 6 years ago

Thanks! Please consider making a release because the port sources it from http://www.met.reading.ac.uk/clouds/adept/ which doesn't get latest pushes.

rjhogan commented 6 years ago

Will do on Monday hopefully - have one further issue to resolve and then need to test with a larger application before I can be sure it's ready for a release.

yurivict commented 6 years ago

Thanks!

djsutherland commented 6 years ago

Support for _mm_undefined_ps was added in clang 3.8, if you wanted to support it there too. (It was added in this commit, which via git branch -a --contains 85a91ffe is present on the release_38 brach.) But actually:

marketing version numbers should not be used to check for language features, as different vendors use different numbering schemes. Instead, use the Feature Checking Macros.

I'm not totally sure how you're supposed to use them in this case, though.

rjhogan commented 6 years ago

I think it can be done with #if __has_builtin(__builtin_ia32_undef128) This seems to work in the latest commit (only tested on Clang >= 3.8, not Clang < 3.8).

rjhogan commented 6 years ago

2.0.5 now released at http://www.met.reading.ac.uk/clouds/adept/

yurivict commented 6 years ago

Thanks!

rjhogan commented 6 years ago

With no reports to the contrary, I will assume that 2.0.5 fixes this issue