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

Doctest + some SIMD BinaryOps via Eigen + compiler target architecture #16

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

chairbender
Copy link

@chairbender chairbender commented Mar 23, 2025

This is a more fully-fledged PR than #15 that I would intend to get merged unless there are large changes needed. Relates to #8 .

This PR:

  1. Adds doctest and the related setup to have them run in CI builds. The tests are inline with the production code. If we don't like this, we can move them to a more "traditional" setup, though I think considering how this project is structured, there are some benefits to doing it this way.
  2. Adds some doctests to test all of the BinaryOp_ structs (NOT the BINOP / UNOP stuff yet! But from my brief check, Eigen should be able to support those various functions).
  3. Adds eigen + ports the BinaryOp code to use it, which should theoretically use SIMD instructions.
  4. Setup the build to enable optimizations so the vectorization capabilities are actually used.
  5. Some updates to the README to fix formatting quirks + describe the new changes to the build system.

The CI has changed slightly:

  1. Tests are now run, and the build will fail if tests fail.
  2. Each of the 3 systems builds for a specific target architecture, which is needed in order for the SIMD stuff to actually be used while still building a binary that should run on many systems. Since we don't distribute the binaries currently, it doesn't really matter, but I used this to illustrate how we could possibly handle this in the future.
    • On the Windows / Linux builder we target x86_64_v2 which should be compatible with 2008-ish or newer CPUs apparently (don't quote me on that number). On OSX it targets M1 or newer.
    • We can of course target multiple architectures if needed in the future.

This affects the following development processes:

  1. When building, you should specify a target and not simply build all targets (meson compile sapf -C build gets you the "native" optimizations for your system). You won't be able to build all the targets.
  2. When testing, you can run tests either against the optimized or unoptimized exe i.e. meson test unoptimized .... The idea being, you can build and test more quickly only by testing the unoptimized exe. Not sure if it's significant enough to be worth it.

This is literally the first time I've ever had to think about compiling for specific architectures and optimization levels so there might be mistakes. I had to research a fair bit and do some trial and error to put this together. It's also the first time I've done anything with SIMD...

I wanted to submit this before porting ALL of the Accelerate code over so you can see how I'm approaching it and I can course-correct before I go too far down the wrong path. Plus there's a lot of Accelerate stuff so it should probably be split into multiple smaller PRs anyway.

Doctest Details

One (pretty minor) downside I am noticing with doctest is this: doctest/doctest#740

Basically, if we have test support stuff (like my CHECK_ARR), we either have to leave it in the release build, or we have to always wrap our doctests themselves PLUS the test support stuff in #ifndef DOCTEST_CONFIG_DISABLE. Because the way doctest generates the tests is such that they are reduced to "no-op" templates, but still must be able to parse. I've opted for the latter approach to minimize test code leaking into production code.

Eigen / SIMD Details

I considered Eigen, SIMDe, XSIMD, and JUCE dsp library as options. Eigen seemed to be the closest to the level of abstraction that we needed (similar to Accelerate) while also having everything we need. XSIMD was the close alternative but it is more "DIY" (we were going to have to deal with alignment and simd size ourselves).

By using the Map type from Eigen, it reads / writes directly from / to the source array rather than having to copy / allocate into an Eigen-specific object. I'm not sure how it deals with alignment internally, but at least on the systems I've run it so far, I'm not seeing alignment assertion errors. If it does become a problem, I think we can specifically use an allocator provided by Eigen.

Once you have the Map instance, you can perform normal operations on it (by using an Array type, they are pairwise rather than matrix / vector algebra).

@chairbender chairbender mentioned this pull request Mar 23, 2025
@chairbender chairbender changed the title Doctest + some SIMD BinaryOps via Eigen Doctest + some SIMD BinaryOps via Eigen + compiler target architecture Mar 23, 2025
@chairbender chairbender marked this pull request as ready for review March 23, 2025 23:42
@chairbender
Copy link
Author

chairbender commented Mar 26, 2025

FYI I ended up getting around to porting over all of the unary operators. But I will keep that separate so this PR doesn't get too big. I will hold off on opening that until we get through this one (and whatever changes are needed). If you want to see what that looks like you can see chairbender#4 . It also shows some more capabilities of doctest such as template tests to avoid duplicating lots of test logic.

(And BinaryOps in chairbender#5 )

@ahihi
Copy link
Owner

ahihi commented Mar 27, 2025

fantastic work, thank you for getting this rolling on both the testing and SIMD fronts! some thoughts:

The tests are inline with the production code. If we don't like this, we can move them to a more "traditional" setup, though I think considering how this project is structured, there are some benefits to doing it this way.

ive been thinking about this and i can see the motivation behind inline tests, but i think i still fall down on the side of having them in separate files. i find this makes it easier to see whether a commit touches program code, tests or both. i havent yet asked JMC whether he would be interested in upstreaming the work in this fork, but if that happens, it seems like separated tests could help reviewability.

When building, you should specify a target and not simply build all targets (meson compile sapf -C build gets you the "native" optimizations for your system). You won't be able to build all the targets.

im not entirely sure what the Meson convention is for this kind of thing, but imo it would be nice if we could have a default build with the native flags without needing to specify a target. maybe this could be set up with build_by_default options?

Basically, if we have test support stuff (like my CHECK_ARR), we either have to leave it in the release build, or we have to always wrap our doctests themselves PLUS the test support stuff in #ifndef DOCTEST_CONFIG_DISABLE. Because the way doctest generates the tests is such that they are reduced to "no-op" templates, but still must be able to parse. I've opted for the latter approach to minimize test code leaking into production code.

i think this is ok, and possibly alleviated by separating out the tests?

@chairbender
Copy link
Author

No problem, I've moved the tests to separate files. It was admittedly kind of a hassle to have them inline and makes it harder to tell what's test code vs. production code. Especially when you start to have non-trivial test harness / test utils you want to include.

I hope using extern to be able to access those ops is fine. It was proving quite difficult for me to get access to those ops through normal means, since they aren't directly exposed via the header.

I updated so the default build is as you indicated. Didn't realize that param existed!

I also refactored the tests based on various things I realized working on my later PRs so there is waaay less code duplication.

The readme is updated to indicate you should use --verbose in order to get test reports when running the meson test command. Otherwise it gets output only to a log file.

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.

2 participants