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.

Sign up for GitHub

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

Jump to bottom

u-c: cabal: add no-msse4.2 flag, enable -msse4.2 as default on x86 architectures #483

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

u-c: cabal: add no-msse4.2 flag, enable -msse4.2 as default on x86 architectures #483

wants to merge 2 commits into from

Conversation

doyougnu
Copy link
Contributor

@doyougnu doyougnu commented Sep 12, 2023 •
edited
Loading

closes #464.

I didn't test this locally so we'll see what CI says.

Also I assumed this would be in a new minor release so I added that to the CHANGE.md file but did not change the versioning in the cabal file

@doyougnu doyougnu force-pushed the doyougnu/add-no-msse-flag branch 2 times, most recently from 757f900 to 45cc38f Compare September 12, 2023 21:26
konsumlamm
konsumlamm reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@konsumlamm konsumlamm left a comment

Choose a reason for hiding this comment

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

I think it would make more sense to call the flag sse42 (the m isn't part of the name, it's just a prefix for machine-specific options) and enable it by default.

unordered-containers.cabal Outdated Show resolved Hide resolved
unordered-containers.cabal Outdated Show resolved Hide resolved
@doyougnu doyougnu force-pushed the doyougnu/add-no-msse-flag branch from 661ec4c to 9a449f2 Compare September 13, 2023 12:56
konsumlamm
konsumlamm reviewed Sep 13, 2023
View reviewed changes
unordered-containers.cabal Outdated Show resolved Hide resolved
unordered-containers.cabal Outdated Show resolved Hide resolved
@doyougnu doyougnu force-pushed the doyougnu/add-no-msse-flag branch from b2d9772 to ea9dd46 Compare September 13, 2023 14:19
@doyougnu doyougnu force-pushed the doyougnu/add-no-msse-flag branch from 1bd9fa3 to ad52f90 Compare September 13, 2023 14:25
@phadej
Copy link
Contributor

phadej commented Oct 10, 2023

I'd suggest OP use some global version of ghc-options: -msse4.2, rather than adding it to individual packages. Adding to packages doesn't scale. I opened a GHC issue https://gitlab.haskell.org/ghc/ghc/-/issues/24073

@TeofilC
Copy link

TeofilC commented Apr 25, 2024

unordered-containers is quite inlining heavy, eg, I often see lookups inlined quite heavily when looking at Core. If the popcount call ends up being inlined when compiling another package (that isn't passed -msse4.2) will this flag still apply?

@phadej
Copy link
Contributor

phadej commented Apr 25, 2024

@TeofilC

will this flag still apply?

Good question. I think it wont, as at Core level (i.e. in unfoldings to be inlined) we only have magic popCount# primop, and the code generation happens later.

@TeofilC TeofilC mentioned this pull request Jun 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konsumlamm konsumlamm konsumlamm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

u-c should use -msse4.2 by default
4 participants
@doyougnu @phadej @TeofilC @konsumlamm