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

disable AES on s390x architecture #3738

Closed
wants to merge 1 commit into from

Conversation

tmh1999
Copy link
Contributor

@tmh1999 tmh1999 commented May 1, 2018

AES is only on available on x86*

Copy link
Collaborator

@moneromooo-monero moneromooo-monero left a comment

Choose a reason for hiding this comment

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

It'd seem better to only enable it on the few archs we can. Let this go in, I'll open a bug for this to change.

@ghost
Copy link

ghost commented May 2, 2018

AES is also available on (some) ARMv8

@hyc
Copy link
Collaborator

hyc commented May 3, 2018

@NanoAkron yes but not using -maes flag.

@vtnerd
Copy link
Contributor

vtnerd commented May 4, 2018

Did a comment get deleted? My email is showing one that isn't coming up ... Anyway cmake has CheckCXXCompilerFlag, CheckCXXSourceCompiles, and CheckCXXSourceRuns. They are going to be tricky to use properly (false positives), so I am not sure if anyone wants to put that much effort into this. But it should be possible.

@tmh1999
Copy link
Contributor Author

tmh1999 commented May 4, 2018

Would you mind merge this PR for now and wait for #3740 later ?

@moneromooo-monero
Copy link
Collaborator

It will be merged once we get to it after we start merging to master again. We will not be waiting for #3740.

@baryluk
Copy link

baryluk commented May 16, 2018

You should test for positive archs, not negative archs. I.e. if you go this route you might need to exclude dozen other archs. Better to list just supported archs. Or just ask compiler if it understands the flag.

@NanoAkron -maes is about AES-NI, it is only on x86 and amd64 archs. and maybe ia64. Many ARM subarchs, and even modern PPC, Power, MIPS and s390x does in fact have AES acceleration one way or another, but it is different thing.

@iamsmooth
Copy link
Contributor

Maybe references to 'AES support' in the CMake should be changed to something better descriptive of what it actually does?

@luigi1111
Copy link
Collaborator

@tmh1999 please rebase.

@moneromooo-monero
Copy link
Collaborator

ping

@moneromooo-monero
Copy link
Collaborator

I rebased this to current master, see #4401.

@fluffypony
Copy link
Contributor

Closed; cherry-picked, rebased and merged via #4401

@fluffypony fluffypony closed this Sep 21, 2018
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.

8 participants