-
Notifications
You must be signed in to change notification settings - Fork 147
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
build: add initial CMake based build system #105
Conversation
I'm somewhat disinclined to take this, because it adds an entirely parallel build system that needs to be kept in sync without tests. I would be somewhat more willing to consider it if it mirrored the way swiftpm builds (describe only dependencies between targets, with automatic discovery of source files within a target dir instead of the CMake style lists), because then there would be less risk of divergence. Not sure how feasible that is, since I am only barely functional in CMake. |
Unfortunately, s-p-m doesn't really function on Windows. The CMake itself does more than s-p-m as it deals with installation as well which is the bulk of the code. The rest does describe dependencies between targets albeit with explicit source listing. The glob based builds are actually rather fragile and exhibit odd behaviours as times :( |
Thanks. What "additional math operations" do we need to build tests? I would be happier with this if we can get testing working. |
The additional math operations seem to be related to |
Ah, and probably __div[sd]c3, too. Those are only used as a benchmarking reference, so you can just #if out those test on Windows. I'm planning to move them out of the test target into a separate benchmark binary anyway, so we can force it to run in Release only; at that point we'll just disable it for Windows (or use the Windows equivalent symbols). |
(That actually sounds like an llvm bug worth fixing; we shouldn't be lowering complex mul/div to a compiler-rt function that doesn't exist on Windows.) |
Yeah, I think this is either a LLVM or a Swift issue that needs investigation. I don't think it makes sense for swift-numerics to special case things - its better to fix it in the compiler. |
Definitely LLVM or Clang. The functions in question are produced by the This should be easy to fix, and seems like a no-brainer. |
Oh, thanks for the pointers - should make it easier for me to track down if its just in LLVM |
Yeah, it should be basically exactly the same type of thing as you fixed for hypotf last year (and all you'll need to reproduce it is |
@stephentyrone I dont happen to see any shims for that, is this not generated by clang/LLVM? |
@compnerd I don't understand the question, but trying to read between the lines: when clang sees a complex multiplication or division (the |
Right, the call generated there is not through the shims but from the compiler lowering the division into a library call. That’s why I was confused how this could be done through the shims. I’m just not sure of I’m missing some clever trick. I recently investigated this a further. There’s the _Cmulcc and _Cmulcr family of functions on Windows for the complex multiply, however there’s no division routine. Further more, I’m not sure if the routines are IEEE-754 compliant. There’s no compiler support for C99 complex in MSVC so I’m a little weary of trying to change clang too aggressively. What do you think of splitting this work into smaller pieces? The first change could allow building the library on windows. I’ll see what we can do on Windows for C99 operations (perhaps we just shim in the clang routines on Windows. Then I can make subsequent changes to enable building the test suite on windows. |
754 says nothing about complex, so no worries there. The applicable standard is C11. We don't really need these for Swift Numerics; they're only used as a baseline for performance measurement. I'll refactor the perf tests into an executable target and the issue will mostly go away. We (the broader LLVM community) should still try to fix the bug, but it would no longer be blocking Swift Numerics (or anything else Swift-related). |
I agree, and I'm willing to go and implement the necessary bits in clang once we have identified what the desired behaviour is. Currently, our guide (MSVC) does not support this functionality (C11 |
If MSVC uses runtime hooks for C++ complex operations, we could investigate using those (after making sure the semantics match with clangs expectations, which I can help with). Or compiler-rt could provide the appropriate symbols on Windows. |
I took a look at what happens with C++ complex options. It seems that it ends up in an open coded division routine: template <class _Other>
void _Div(const complex<_Other>& _Right) {
using _Myctraits = _Ctraits<_Ty>;
_Ty _Rightreal = static_cast<_Ty>(_Right.real());
_Ty _Rightimag = static_cast<_Ty>(_Right.imag());
if (_Myctraits::_Isnan(_Rightreal) || _Myctraits::_Isnan(_Rightimag)) { // set NaN result
this->_Val[_RE] = _Myctraits::_Nanv();
this->_Val[_IM] = this->_Val[_RE];
} else if ((_Rightimag < 0 ? -_Rightimag : +_Rightimag)
< (_Rightreal < 0 ? -_Rightreal : +_Rightreal)) { // |_Right.imag()| < |_Right.real()|
_Ty _Wr = _Rightimag / _Rightreal;
_Ty _Wd = _Rightreal + _Wr * _Rightimag;
if (_Myctraits::_Isnan(_Wd) || _Wd == 0) { // set NaN result
this->_Val[_RE] = _Myctraits::_Nanv();
this->_Val[_IM] = this->_Val[_RE];
} else { // compute representable result
_Ty _Tmp = (this->_Val[_RE] + this->_Val[_IM] * _Wr) / _Wd;
this->_Val[_IM] = (this->_Val[_IM] - this->_Val[_RE] * _Wr) / _Wd;
this->_Val[_RE] = _Tmp;
}
} else if (_Rightimag == 0) { // set NaN result
this->_Val[_RE] = _Myctraits::_Nanv();
this->_Val[_IM] = this->_Val[_RE];
} else { // 0 < |_Right.real()| <= |_Right.imag()|
_Ty _Wr = _Rightreal / _Rightimag;
_Ty _Wd = _Rightimag + _Wr * _Rightreal;
if (_Myctraits::_Isnan(_Wd) || _Wd == 0) { // set NaN result
this->_Val[_RE] = _Myctraits::_Nanv();
this->_Val[_IM] = this->_Val[_RE];
} else { // compute representable result
_Ty _Tmp = (this->_Val[_RE] * _Wr + this->_Val[_IM]) / _Wd;
this->_Val[_IM] = (this->_Val[_IM] * _Wr - this->_Val[_RE]) / _Wd;
this->_Val[_RE] = _Tmp;
}
}
} It would be no worse to just pull in the implementation from compiler-rt I think. |
This should be just a matter of turning on building those functions for Windows in the compiler-rt build, then, right? |
Not really - the thing is that (Adding that isn't something that is entirely viable because clang attempts to behave like MSVC on Windows) |
In which library, though? We don't need these for Swift, so putting them in Swift Numerics or the Swift runtime library makes no sense. We need them for C, and the runtime support library for C (in clang) is compiler-rt. The other alternative, taking "behave like MSVC" to its logical conclusion, would be to disable support on Windows for any C feature that MSVC doesn't support (including _Complex), but that seems drastic. |
That is the challenge, |
I’m a little confused by this. This is a clang issue, not swift. Compiler-rt is the numerics support library for clang. |
The problem is that clang is trying to run in a mode where its emulating the behaviour of MSVC intentionally. That basically means that if you want to link against compiler-rt, you are on your own (as in you need to tell the linker that it needs to be linked against). Basically, what I am suggesting that we have a few different options:
Part of the problem is that the builtins are not setup for the MSVC mode, so there will be some more work there as well. The shims would be easier is all. |
Right, but options 1 and 2 make no sense, because Swift doesn't need these operations, C does. Putting them in swift numerics or swiftCore doesn't do anything to address the problem. Option 3 also makes no sense, because that shim would go ... where? It doesn't matter, because if it existed, it would force auto-linking for C code, which would be the same as just ... linking it for C code. Fundamentally, the question is this--should the following valid C code compile and run with clang on Windows: _Complex double foo(_Complex double z, _Complex double w) { return z*w; } If the answer is yes, then clang should be linking (a possibly stripped-down) compiler-rt on Windows, because the necessary builtin isn't available anywhere else. If the answer is no, then clang should disable support for _Complex in the front end when targeting Windows. These are really the only two options--note that Swift doesn't enter into it at all, so there's no possibility of fixing this via swiftCore or swift numerics. |
I think that getting that change into upstream clang would take a while to do since the default behaviour is to be compatible with the de facto platform standard (MSVC). It may be possible to add a number of flags to control the behaviour in the extension space (and I'm worried that people are going to bring back the long time desire for |
The pragmatic solution for S-N is "just don't build those benchmarks on Windows," as there's no point in trying to compare ourselves against C functionality that doesn't exist. |
@stephentyrone I think that actually makes a lot more sense, thanks for talking through this :) |
Okay, I think that we have a good approach for the dependency issue - so the next question is about getting there. What do you think of splitting up the work, get it building and then do a follow up for the testing? |
@stephentyrone - okay, tested it locally now.
Everything seems good now on Windows! |
@stephentyrone - gentle reminder about this :) |
@stephentyrone - done, fixed header on the |
This adds a CMake based build, which enables the build of swift-numerics on Windows. This currently does not add the test suite to the build as there are still some issues to work out in building the full test suite which require additional math operations to fully build.
Gentle ping on this. |
I'm still mildly concerned about the fragility of having a second build system that isn't actually used by most contributors to or users of a project. In particular I would like to have a clear policy about what the responsibilities for contributors testing and updating the CMake build should be. What would you like to see? |
I think you've resolved any technical concerns that I had, I just want to establish clear guidance for contributors here before landing this. |
I completely understand that concern. I’m planning on adding the ability to build it with build script optionally through CMake. I think that just updating the list when new files are added would be appreciated, but I do understand that it can break from time to time. It’s usually pretty easy to repair when noticed (argument parser does the same thing). It is possible to add a GH actions based build with CMake if there’s interest in that as well. |
build: add initial CMake based build system
This adds a CMake based build, which enables the build of swift-numerics
on Windows. This currently does not add the test suite to the build as
there are still some issues to work out in building the full test suite
which require additional math operations to fully build.