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

feat: avx feature switch #1034

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

Conversation

ohaiibuzzle
Copy link
Member

Apple: We add a feature!
Also Apple: We also make it 10 layers deep inside the Rosetta 2 binary locked behind an environment variable, good luck!

(Although I think some people found out that AVX support may be incomplete)

@Gcenx
Copy link

Gcenx commented Jun 18, 2024

As this environment variable isn’t documented there’s no telling if/when it’ll be removed.

Also should add a warning about this option as enabling it would cause AVX/2 to be used over a possible SSE2 code path, the AVX/2 code path would be slower than SSE2.

@ohaiibuzzle
Copy link
Member Author

@Gcenx Added a bit of warning for that. We can just remove it if it's no longer needed later
image

@Gcenx
Copy link

Gcenx commented Jun 18, 2024

@Gcenx Added a bit of warning for that. We can just remove it if it's no longer needed later image

Remove emulator wine isn’t emulation

This seems more reasonable

This option may significantly impact performance.

@ohaiibuzzle
Copy link
Member Author

ohaiibuzzle commented Jun 18, 2024

Remove emulator wine isn’t emulation

Wine isn't, but Rosetta is, since it's the one doing x86_64 translation.
So I think using the term emulation might be the correct one here, as it is a R2 environment variable.

@Gcenx
Copy link

Gcenx commented Jun 18, 2024

I’d still remove the term emulation and instead change AVX

AVX support (emulated)

That’s at least then more accurate.

@ohaiibuzzle
Copy link
Member Author

Then probably the best middleground (and to keep it short) would be

AVX Emulation
⚠️ Significantly impacts performance

@Gcenx
Copy link

Gcenx commented Jun 18, 2024

That seems the best compromise

@ultratiem
Copy link

ultratiem commented Jun 19, 2024

“Significantly impacts performance” an impact is merely a strong effect, so significant becomes redundant (like wet water). It doesn’t inherently mean good or bad, so if you want to add a warning that it may adversely affect something, you need to be explicit:

“May negatively impact performance”

From a UX standpoint, you don’t need to scare users. The warning emoji for instance, one wonders why they would ever turn it on let alone why it was even added. Thus it may be better form to simply add (experimental) to the label:

AVX (Experimental)
May negatively impact performance

This lets users know it’s not a feature but simply a testing ground.

@Gcenx
Copy link

Gcenx commented Jun 19, 2024

I disagree the current label and text, maybe minus the icon ensure users will understand turning that option on will have side-effects.

It’s not grammatically incorrect.

Not making this clear will result in it being used like Retina, where users enable it then complain.

@ohaiibuzzle
Copy link
Member Author

ohaiibuzzle commented Jun 20, 2024

@ultratiem I mean, maybe we could brainstrorm something out, but, just fyi
This is how Apple's own goddang OS does it for "side effects":

image

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.

None yet

3 participants