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

Add support for 32 bit architectures #67

Merged
merged 2 commits into from
Dec 24, 2022
Merged

Add support for 32 bit architectures #67

merged 2 commits into from
Dec 24, 2022

Conversation

erikd
Copy link
Owner

@erikd erikd commented Aug 14, 2022

Closes: #36

@erikd
Copy link
Owner Author

erikd commented Aug 14, 2022

I have tried building this in a 32 bit chroot (with ghc 9.0.2 and cabal-install 3.4.1.0), but a number of the dependencies don't build.

Even building with tests disabled, base-orphans and primitive fail to build.

Related: haskell/cabal#6602

@Bodigrim
Copy link
Contributor

Probably haskell/actions/setup@v1 cannot be used on i386. I'd try sticking closer to https://github.com/haskell/filepath/blob/8366ecedc473c42aae13af38fbb3aa4131917aa0/.github/workflows/test.yaml#L122-L141, especially

    - name: install
      run: |
          apt-get update -y
          apt-get install -y ghc libghc-quickcheck2-dev libghc-tasty-dev libghc-tasty-quickcheck-dev git make curl libghc-exceptions-dev

run: |
cabal --version
cabal update
cabal configure --enable-tests --constraint="quickcheck-classes -aeson -semigroupoids -vector"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try

Suggested change
cabal configure --enable-tests --constraint="quickcheck-classes -aeson -semigroupoids -vector"
cabal configure --enable-tests --constraint="quickcheck-classes -aeson -semigroupoids -vector" --constraint "semirings -unordered-containers"

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can probably remove all the constraints. Just do not want to make too many big changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you actually want to add constraints, not remove them :)
The goal is to build as little as possible to avoid intermittent fdTryLock issues.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to upgrade the cabal-install version to one where the fdTryLock issue is fixed.

@iliastsi
Copy link

@erikd I tried this PR on i386. I had to change word64TofromWord# to word64ToWord#. The module now compiles (yay!) but the tests fail. Here is an example:

  ✗ prop_bitwise_xor failed at test/Test/Data/WideWord/Word128.hs:178:28
    after 1 test and 2 shrinks.                                                                
                                               
        ┏━━ test/Test/Data/WideWord/Word128.hs ━━━
    174 ┃ prop_bitwise_xor :: Property
    175 ┃ prop_bitwise_xor =            
    176 ┃   propertyCount $ do               
    177 ┃     (a, b) <- H.forAll $ (,) <$> genWord128 <*> genWord128
        ┃     │ ( 0 , 340282366920938463444927863358058659840 )
    178 ┃     toInteger128 (xor a b) === xor (toInteger128 a) (toInteger128 b)
        ┃     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                 
        ┃     │ ━━━ Failed (- lhs) (+ rhs) ━━━                                                 
        ┃     │ - 79228162495817593519834398720 
        ┃     │ + 340282366920938463444927863358058659840
                                               
    This failure can be reproduced by running: 
    > recheck (Size 0) (Seed 8870710184482104703 4727740369842831805) prop_bitwise_xor

and

  ✗ prop_divMod failed at test/Test/Data/WideWord/Word128.hs:292:38
    after 3 tests and 130 shrinks.
  
        ┏━━ test/Test/Data/WideWord/Word128.hs ━━━
    286 ┃ prop_divMod :: Property
    287 ┃ prop_divMod =
    288 ┃   propertyCount $ do
    289 ┃     num <- H.forAll genWord128
        ┃     │ 79228160724930162448012410879
    290 ┃     den <- H.forAll $ Gen.filter (/= 0) genWord128
        ┃     │ 79228160669589930226883756032
    291 ┃     let (d, m) = divMod num den
    292 ┃     (toInteger128 d, toInteger128 m) === divMod (toInteger128 num) (toInteger128 den)
        ┃     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        ┃     │ ━━━ Failed (- lhs) (+ rhs) ━━━
        ┃     │ - ( 1 , 55340232225423622143 )
        ┃     │ + ( 1 , 55340232221128654847 )
  
    This failure can be reproduced by running:
    > recheck (Size 2) (Seed 9021709147519205028 426905579932177907) prop_divMod

I can provide the whole list of tests that fail, if that would help.

@erikd
Copy link
Owner Author

erikd commented Oct 2, 2022

@iliastsi That change seems to make sense. Would you be able to post a git patch here in the comments or submit a PR against this branch?

However, the main issue is that I cannot seem to get i386 CI working for this package. Since I do all my dev on x86_64 I really do need a working reliable i386 CI. Getting i386 CI working is actually a higher priority that fixing this bug :/ .

@swt2c
Copy link

swt2c commented Oct 13, 2022

Hi @erikd, I submitted a PR (#70) to this branch that gets the i386 CI working (just switch to Debian instead of Ubuntu - Ubuntu has dropped most i386 support) and applies @iliastsi suggestions. You should be able to see the same test failures now.

@swt2c
Copy link

swt2c commented Nov 8, 2022

Hi @erikd, any chance to look at these test failures?

@erikd
Copy link
Owner Author

erikd commented Nov 8, 2022

Sorry, escaped my notice. Will look at this some time between now and Monday.

@erikd
Copy link
Owner Author

erikd commented Nov 21, 2022

Dang, forgot about this weekends in a row.

@iliastsi
Copy link

@erikd Any update on this? It would be great if we can fix it in time for the next version of Debian.

@erikd
Copy link
Owner Author

erikd commented Dec 16, 2022

@iliastsi I use and love Debian, but I see little point in doing this just so the package is available for a little used architecture in Debian.

Fixing this, and making the code compile and pass all tests on 32 bit and 64 bit systems is rather difficult.

@iliastsi
Copy link

Fixing this, and making the code compile and pass all tests on 32 bit and 64 bit systems is rather difficult.

I understand. Unfortunately, there are other packages that use wide-word in Debian now, which means that those packages are blocked as well (all the way up to taffybar). If we cannot fix wide-word to work on 32 bit systems, I will request that these packages get removed from those architectures.

@erikd
Copy link
Owner Author

erikd commented Dec 19, 2022

Unfortunately fixing this requires an enormous amount of work.

The underlying problem is that Word# is an unboxed native word, ie 64 bits on a 64 bit system and 32 bits on a 32 bit system and that all the low level primitive operations (like plus, minus, and or etc) are defined on the native word sized Word#. To make this an even bigger pain in the neck, the Word64 type is not even available on 32 bit systems 😢 .

Therefore there are a couple of possible fixes:

  • Rewrite all the existing implementation in terms of word size independent Word64. This type is a boxed type and therefore we would be relying on the compiler to do the required boxing and unboxing to keep up the performance on `x86_64. Obviously some careful benchmarking would be required to show that any performance degradation is minimal at most. The other problem with this solution is that things like adding or multiplying two unboxed words with a result and a carry are not supported.
  • Write a full 32 bit implementation of the three existing data types and then use CPP an WORD_SIZE_IN_BITS to switch between them. This significantly increases the maintenance burden.
  • Switch to a generic WordArray# based implementation, that would then be specialized for the three existing data types and two different word sizes (32 and 64 bits).

I am currently leaning towards the first, but that would require the implementation of a Word64 type for 32 bit systems.

My real feeling is, why can't 32 bit systems just die.

@erikd
Copy link
Owner Author

erikd commented Dec 20, 2022

I have done a huge chunk of the first option and all tests pass in 64 bits, but there are a number of failings tests of 32 bits.

@erikd
Copy link
Owner Author

erikd commented Dec 21, 2022

Ran into a GHC bug: https://gitlab.haskell.org/ghc/ghc/-/issues/22654 (which then turned out not to be a bug).

@erikd erikd force-pushed the erikd/32-bit branch 3 times, most recently from f6d33f6 to fb12338 Compare December 24, 2022 08:10
@erikd
Copy link
Owner Author

erikd commented Dec 24, 2022

@iliastsi This is failing in CI on emulated-i386 but passes for me locally on 32 bits using ghc-9.2.5. It does not work with versions of GHC earlier that 9.2.1 because the code uses GHC primops that are not available in earlier versions.

This requires:
* Addition of Data.WideWord.Word64 top support 32 bit systems (which do
  not have a Word64 type) as well as primitives that do addition,
  subraction and multiplication with carry on unboxed values.
* Implement all operations on Int128, Word126, Word256 etc in terms of
  Data.WideWord.Word64 so that these work on 32 bit systems.
Drop CI build with ghc-8.0 and ghc-8.2 because CI can't find them
anymore.

Run emulated-i386 tests on Debian Bookworm.
@erikd erikd merged commit 8c13558 into master Dec 24, 2022
@erikd erikd deleted the erikd/32-bit branch December 24, 2022 21:36
@iliastsi
Copy link

@iliastsi This is failing in CI on emulated-i386 but passes for me locally on 32 bits using ghc-9.2.5. It does not work with versions of GHC earlier that 9.2.1 because the code uses GHC primops that are not available in earlier versions.

@erikd Awesome work, thanks for this! Looking forward to update to GHC 9.2.1 in Debian.

@erikd
Copy link
Owner Author

erikd commented Dec 28, 2022

Upstream has already released ghc-9.2.5. I would highly recommend using that instead of 9.2.1.

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.

Compliation for ARM fails
4 participants