lemire / FastPFor

The FastPFOR C++ library: Fast integer compression
Apache License 2.0
883 stars 124 forks source link

Add ARM Support for FastPFOR using SIMDe #108

Closed seb711 closed 10 months ago

seb711 commented 11 months ago

This pull request introduces support for ARM architecture in the FastPFOR project, particularly for building on aarch64 devices. The need for ARM support arose while running FastPFOR on a Graviton Instance. After conducting research and consulting with my mentor at Technical University of Munich (TUM), we decided to integrate SIMDe.

This is a draft pull request, primarily to seek feedback and further insights. The current state of the code might not fully align with the repository's requirements, and due to time constraints, I couldn't perform exhaustive tests (FastPFOR_unittest and "make check" run without errors though). The purpose of this pull request is to share the progress with the community, especially for those interested in building the FastPFOR library on an AARCH64 machine.

lemire commented 11 months ago

Sure, this can be merged, if the tests are passing.

Please see the failing tests.

lemire commented 11 months ago

@seb711 I invite you to make this PR as ready for review. We should also add ARM tests in CI, I will add another comment.

lemire commented 11 months ago

@seb711 As part of this PR, can you add in .github/workflow a file that could be named vs17-arm-ci.yml (or anything you like).

It should have content like so...

name: VS17-ARM-CI

on: [push, pull_request]

jobs:
  ci:
    name: windows-vs17
    runs-on: windows-latest
    strategy:
      fail-fast: false
      matrix:
        include:
          - {gen: Visual Studio 17 2022, arch: ARM64}
    steps:
      - name: checkout
        uses: actions/checkout@v4
      - name: Use cmake
        run: |
          cmake -G "${{matrix.gen}}" -A ${{ matrix.arch }} -DCMAKE_CROSSCOMPILING=1 -B build  &&
          cmake --build build --verbose

This will only check that we can build for ARM under Visual Studio. It is kind of a sanity test.

lemire commented 11 months ago

@seb711 Please review a PR that I have issued against your own PR :

https://github.com/seb711/FastPFor/pull/1

Note that I am doing a PR against your own PR, which you should merge if you approve. Then we can go back and merge your PR into lemire/FastPFor.

I think your code is sane and I will merge. Good work !!!

seb711 commented 10 months ago

Sorry for the major delay (had some busy days at uni and thought this PR would require more work ^^); added the workflow file and merged your branch.

lemire commented 10 months ago

Running tests.

lemire commented 10 months ago

@seb711 Can you have a quick look? Seems like it does not build for ARM...

seb711 commented 10 months ago

Hi, so I had only tested it for GCC and ARM and not VS-Compiler and ARM. Therefore I had to add the predefined compiler flags for MSC. Added the VS-ARM Workflow + Tests to the PR, so that the ARM build gets also tested.

Sorry for the delay, but i needed an alternative implementation for the rdtsc-call in MSC+ARM (couldn't use asm volatile)

lemire commented 10 months ago

@seb711 Ok. We can merge. Please confirm that you are happy with the PR as it stands.

seb711 commented 10 months ago

@lemire I'm happy with the PR as it stands by now (also worked with it in the last weeks and experienced no issues). I only have one concern: Until there is no Github Runner, that tests ARM runs with GCC, any future change related to ARM builds with GCC cannot be verified with the runners. Arm Runner are announced (https://github.blog/changelog/2023-10-30-accelerate-your-ci-cd-with-arm-based-hosted-runners-in-github-actions/), but are only in private beta by now .

lemire commented 10 months ago

I am merging.