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

Compilation fixes #70

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

Conversation

bjourne
Copy link

@bjourne bjourne commented Jun 11, 2016

A few little compiler warnings cleanups and makefile improvements. I also think you should consider changing -O2 to -O3 as that makes the c_fast benchmark much faster when compiled with clang. Clang in general beats gcc on my machine.

@@ -1,21 +1,25 @@
NUM_NODES = 10
WORLD_SIZE = 1000

COMMON_CFLAGS = -g -std=gnu99 -O2 -mcpu=native -fomit-frame-pointer -Wall -Wextra
COMMON_CFLAGS = -std=gnu99 -O2 -march=native -fomit-frame-pointer -Wall -Wextra
Copy link
Contributor

Choose a reason for hiding this comment

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

-march=native is not equivalent to -mcpu=native. It needs to be -march=native -mtune=native.

Copy link
Author

@bjourne bjourne Jun 11, 2016

Choose a reason for hiding this comment

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

Copy link
Contributor

@lgeek lgeek Jun 11, 2016

Choose a reason for hiding this comment

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

It's not documented to work like that in the GCC ARM docs. Also, the options accepted by -march are generic architectures, while the options accepted by -mtune are specific cores.

Copy link
Author

Choose a reason for hiding this comment

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

But previously the makefile didn't even specify -mtune. -march at least implies -mcpu.

@bjourne
Copy link
Author

bjourne commented Jun 11, 2016

@lgeek, I apologise for not making myself more clear. My intention with this pr was not to invest to much time in your LPathBench project. I just wanted to share the changes I needed to make it work well for me. Feel free to reject this pr, apply it or do whatever you want with it. Sorry, but I don't have either the time or the interest in working more on it.

@lgeek
Copy link
Contributor

lgeek commented Jun 11, 2016

@bjourne Sorry if this is wasn't clear, but this is not my project. I just wrote the c-fast implementation and I've only reviewed the patches related to it. It's not up to me what gets merged. I've pointed out the changes which either break things (e.g. the printf one) or which change behavior (the flags).

@bjourne
Copy link
Author

bjourne commented Jun 11, 2016

I think "%"PRIu64 is the right way to express the format. Though it appears that %lu does not break anything on 32bit platforms.

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.

2 participants