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

Use byteSwapX# in WORDS_BIGENDIAN #260

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Conversation

sgraf812
Copy link
Collaborator

Spurred by @Bodigrim in haskell/happy#280 (comment), let's try having a bit more efficient code on WORD_BIGENDIAN architectures.

Mostly uploading this for CI, hoping there will be a big-endian machine to test the new code path.

@sgraf812
Copy link
Collaborator Author

This seems to work as expected, presuming the MacOS build really tests the WORDS_BIGENDIAN code path. And I couldn't tell how it would work otherwise.

@sgraf812 sgraf812 changed the title Draft: Use byteSwapX# in WORDS_BIGENDIAN Use byteSwapX# in WORDS_BIGENDIAN Jun 26, 2024
@andreasabel
Copy link
Member

Thanks!

Could we get a changelog entry for this?

@sgraf812
Copy link
Collaborator Author

Done. I say 3.5.2.0 in the CHANGELOG, but I'm inclined to think it's only a PATCH change really. You can always adjust later, I guess.

@Bodigrim
Copy link

This seems to work as expected, presuming the MacOS build really tests the WORDS_BIGENDIAN code path.

Unfortunately it does not, Apple hardware is little endian for the past 15 years or so.

To test the change properly one has to setup an emulated job for s390x architecture (aka IBM mainframe). There are several examples of such CI setup around github.com/haskell, the most recent one I worked on is https://github.com/haskell/tar/blob/master/.github/workflows/emulated.yml.

@sgraf812
Copy link
Collaborator Author

Oh, wow... I somehow thought that iOS was big endian, but it actually isn't. Nevermind then.

data/AlexTemplate.hs Outdated Show resolved Hide resolved
@sgraf812 sgraf812 force-pushed the wip/sgraf branch 7 times, most recently from 569fb3f to cf6599f Compare July 12, 2024 18:47
@sgraf812
Copy link
Collaborator Author

sgraf812 commented Jul 12, 2024

Alright, I added an emulated CI pipeline as @Bodigrim suggested. Not pretty, but does the job, and shouldn't break all to often given that alex is pretty stable.

For the job in https://github.com/sgraf812/alex/actions/runs/9912679649/job/27388017292 (569fb3f), I replaced byteSwap with garbage to test that the WORDS_BIGENDIAN code path is covered, so that job is expected to fail.

The job in https://github.com/sgraf812/alex/actions/runs/9912681641/job/27388023583 (36cc05e) does no such replacement and consequently is green.

No idea why the other CI jobs fail cabal check.
Edit: It sure is suspicious that all (and only) jobs from the haskell-ci-simple.yml workflow fail.
Edit2: I see similar errors in the regenerated CI workflows of happy. Sigh. It really seems that all deps need to announce upper bounds now.

@andreasabel
Copy link
Member

It really seems that all deps need to announce upper bounds now.

No, I think only base absolutely needs one now:

Error: [missing-bounds-important] The dependency 'build-depends: base' does not specify an upper bound on the version number.

@andreasabel
Copy link
Member

I am fixing Haskell CI in this PR that we should merge first:

@andreasabel
Copy link
Member

@Mergifyio rebase

sgraf812 and others added 2 commits July 14, 2024 23:16
@andreasabel
Copy link
Member

@sgraf812 : Do you want this change released now?

@sgraf812
Copy link
Collaborator Author

sgraf812 commented Jul 15, 2024

Thanks! No need for a release, I just wanted to test the code in haskell/happy#280. Replicating the emulated-CI-without-cabal setup for the multi-component happy project is far too ambitious for my tastes.

Edit: Do press merge when you think this is ready!

@andreasabel andreasabel merged commit 3f7fc0c into haskell:master Jul 15, 2024
23 checks passed
@andreasabel andreasabel added this to the 3.5.2.0 milestone Jul 15, 2024
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