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

build: add initial CMake based build system #105

Merged
merged 2 commits into from
Jul 3, 2020

Conversation

compnerd
Copy link
Contributor

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.

@stephentyrone
Copy link
Member

stephentyrone commented Mar 20, 2020

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.

@compnerd
Copy link
Contributor Author

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 :(

@stephentyrone
Copy link
Member

stephentyrone commented Mar 20, 2020

Thanks. What "additional math operations" do we need to build tests? I would be happier with this if we can get testing working.

@compnerd
Copy link
Contributor Author

The additional math operations seem to be related to __muldc3 and __mulsc3 IIRC. I want to try to dig into that and replace them with _Cmulcc/_FCmulcc if possible.

@stephentyrone
Copy link
Member

stephentyrone commented Mar 20, 2020

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).

@stephentyrone
Copy link
Member

(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.)

@compnerd
Copy link
Contributor Author

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.

@stephentyrone
Copy link
Member

stephentyrone commented Mar 20, 2020

Definitely LLVM or Clang. The functions in question are produced by the * and / operators in the C shims; Swift-Numerics doesn't use them at all, because we have our own multiply and divide implementations.

This should be easy to fix, and seems like a no-brainer.

@compnerd
Copy link
Contributor Author

Oh, thanks for the pointers - should make it easier for me to track down if its just in LLVM

@stephentyrone
Copy link
Member

stephentyrone commented Mar 20, 2020

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 _Complex double foo(_Complex double x, _Complex double y) { return x*y; }

@compnerd
Copy link
Contributor Author

compnerd commented Apr 17, 2020

@stephentyrone I dont happen to see any shims for that, is this not generated by clang/LLVM?

@stephentyrone
Copy link
Member

stephentyrone commented Apr 22, 2020

@compnerd I don't understand the question, but trying to read between the lines: when clang sees a complex multiplication or division (the * or / operators), it emits a call to __muldc3 or __divdc3 to compute the result (after first attempting an inline computation, in the case of multiply). See: https://godbolt.org/z/zgaBGB

@compnerd
Copy link
Contributor Author

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.

@stephentyrone
Copy link
Member

stephentyrone commented Apr 23, 2020

Further more, I’m not sure if the routines are IEEE-754 compliant.

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).

@compnerd
Copy link
Contributor Author

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 _Complex) so we need to identify the behaviour to implement.

@stephentyrone
Copy link
Member

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.

@compnerd
Copy link
Contributor Author

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.

@stephentyrone
Copy link
Member

This should be just a matter of turning on building those functions for Windows in the compiler-rt build, then, right?

@compnerd
Copy link
Contributor Author

compnerd commented Apr 28, 2020

Not really - the thing is that libclang_rt.builtins-... is not linked against. That needs to be explicitly found and added to the final link command by the user. The easiest thing to do really would be to vend that internally in the library sadly.

(Adding that isn't something that is entirely viable because clang attempts to behave like MSVC on Windows)

@stephentyrone
Copy link
Member

stephentyrone commented Apr 28, 2020

The easiest thing to do really would be to vend that internally in the library sadly.

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.

@compnerd
Copy link
Contributor Author

That is the challenge, clang behaves as cl and in that case, the runtime support is vcruntime which we don't really control. I agree that disabling _Complex in C is definitely drastic. If we don't want to provide it in the numerics support then I suppose that we can require that the user explicitly adds the library.

@stephentyrone
Copy link
Member

If we don't want to provide it in the numerics support

I’m a little confused by this. This is a clang issue, not swift. Compiler-rt is the numerics support library for clang.

@compnerd
Copy link
Contributor Author

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:

  1. provide the shims in swift-numerics
  2. provide the shims in swiftCore
  3. create a simple shim that forces auto-linking of clang_rt.builtins-x86_64.lib
  4. leave it to the user to add the link

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.

@stephentyrone
Copy link
Member

stephentyrone commented Apr 29, 2020

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.

@compnerd
Copy link
Contributor Author

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 -fclang-extensions). Im hoping for a more pragmatic solution here.

@stephentyrone
Copy link
Member

Im hoping for a more pragmatic solution here.

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.

@compnerd
Copy link
Contributor Author

@stephentyrone I think that actually makes a lot more sense, thanks for talking through this :)

@compnerd
Copy link
Contributor Author

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
Copy link
Member

#116

@compnerd
Copy link
Contributor Author

compnerd commented Jun 7, 2020

@stephentyrone - okay, tested it locally now.

S:\BinaryCache\Release\Windows-x86_64\swift-numerics> S:\BinaryCache\Release\Windows-x86_64\swift-numerics\bin\SwiftNumericsTestRunner.exe
Test Suite 'All tests' started at 2020-06-06 18:16:52.191
Test Suite 'bin.xctest' started at 2020-06-06 18:16:52.207
Test Suite 'ElementaryFunctionChecks' started at 2020-06-06 18:16:52.207
Test Case 'ElementaryFunctionChecks.testFloat' started at 2020-06-06 18:16:52.207
Test Case 'ElementaryFunctionChecks.testFloat' passed (0.0 seconds)
Test Case 'ElementaryFunctionChecks.testDouble' started at 2020-06-06 18:16:52.207
Test Case 'ElementaryFunctionChecks.testDouble' passed (0.0 seconds)
Test Suite 'ElementaryFunctionChecks' passed at 2020-06-06 18:16:52.207
         Executed 2 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds
Test Suite 'IntegerExponentTests' started at 2020-06-06 18:16:52.207
Test Case 'IntegerExponentTests.testFloat' started at 2020-06-06 18:16:52.207
Test Case 'IntegerExponentTests.testFloat' passed (0.0 seconds)
Test Case 'IntegerExponentTests.testDouble' started at 2020-06-06 18:16:52.207
Test Case 'IntegerExponentTests.testDouble' passed (0.0 seconds)
Test Suite 'IntegerExponentTests' passed at 2020-06-06 18:16:52.207
         Executed 2 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds
Test Suite 'ArithmeticTests' started at 2020-06-06 18:16:52.207
Test Case 'ArithmeticTests.testPolar' started at 2020-06-06 18:16:52.207
Test Case 'ArithmeticTests.testPolar' passed (0.0 seconds)
Test Case 'ArithmeticTests.testBaudinSmith' started at 2020-06-06 18:16:52.207
Test Case 'ArithmeticTests.testBaudinSmith' passed (0.0 seconds)
Test Case 'ArithmeticTests.testDivisionByZero' started at 2020-06-06 18:16:52.207
Test Case 'ArithmeticTests.testDivisionByZero' passed (0.0 seconds)
Test Suite 'ArithmeticTests' passed at 2020-06-06 18:16:52.207
         Executed 3 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds
Test Suite 'PropertyTests' started at 2020-06-06 18:16:52.207
Test Case 'PropertyTests.testProperties' started at 2020-06-06 18:16:52.222
Test Case 'PropertyTests.testProperties' passed (0.0 seconds)
Test Case 'PropertyTests.testEquatableHashable' started at 2020-06-06 18:16:52.222
Test Case 'PropertyTests.testEquatableHashable' passed (0.0 seconds)
Test Case 'PropertyTests.testCodable' started at 2020-06-06 18:16:52.222
Test Case 'PropertyTests.testCodable' passed (0.0 seconds)
Test Suite 'PropertyTests' passed at 2020-06-06 18:16:52.222
         Executed 3 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds
Test Suite 'bin.xctest' passed at 2020-06-06 18:16:52.222
         Executed 10 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds
Test Suite 'All tests' passed at 2020-06-06 18:16:52.222
         Executed 10 tests, with 0 failures (0 unexpected) in 0.0 (0.0) seconds

Everything seems good now on Windows!

@compnerd
Copy link
Contributor Author

@stephentyrone - gentle reminder about this :)

@compnerd
Copy link
Contributor Author

@stephentyrone - done, fixed header on the WindowsMain.swift and added the copyright headers on the CMakeLists.

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.
@compnerd
Copy link
Contributor Author

Gentle ping on this.

@stephentyrone
Copy link
Member

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?

@stephentyrone
Copy link
Member

I think you've resolved any technical concerns that I had, I just want to establish clear guidance for contributors here before landing this.

@compnerd
Copy link
Contributor Author

compnerd commented Jul 1, 2020

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.

@stephentyrone stephentyrone merged commit 110beef into apple:master Jul 3, 2020
stephentyrone added a commit that referenced this pull request Jul 3, 2020
build: add initial CMake based build system
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.

3 participants