build improvements for 0.16 #19
Conversation
| endif | ||
| ifndef AMD_APP_LIB | ||
| AMD_APP_LIB = | ||
| endif |
There was a problem hiding this comment.
Eventually we might want to use vendor-neutral names like EXTRA_CFLAGS and EXTRA_LIBS and only leave support for the AMD names for compatibility. It can be done separately.
There was a problem hiding this comment.
I'll mentally add that to my list of to-do's for the future.
| LD = $(CPP) | ||
| LDFLAGS = $(ARCHFLAGS) $(BITS) $(STATIC) $(OPTIMIZE_FLAG) $(AMD_APP_LIB) $(OPENCL_LIB) | ||
|
|
||
| CC_VERSION = $(shell $(CC) --version) | ||
|
|
||
| # optimize or debug | ||
| ifneq (, $(findstring gcc, $(CC_VERSION))) | ||
| OPTIMIZE_FLAG = -O3 -funroll-loops -ffast-math -finline-functions -frerun-loop-opt -fgcse-sm -fgcse-las -flto | ||
| OPTIMIZE_FLAG = -O3 -funroll-loops -ffast-math -finline-functions -frerun-loop-opt -fgcse-sm -fgcse-las -flto=auto -save-temps |
There was a problem hiding this comment.
-save-temps saves the preprocessor output and the assembly. That uses extra disk space. I don't think it should be done except when debugging a specific problem.
There was a problem hiding this comment.
I noticed that flto=auto resolves the "using serial compilation of [number] LTRANS jobs" warning but also causes an error saying "auto" is an invalid value. However, adding -save-temps prevents this error. Do you have any other solutions?
There was a problem hiding this comment.
@proski I did some more testing and found that -save-temps generates .i and .ii files. However, the largest one is only around 2.5 MB in size. So I don't think it's that much of a difference.
src/kbhit.cpp
Outdated
| } | ||
| else { | ||
| ssize_t temp = read(0, &ch, 1); | ||
| (void)temp; // avoid warning about unused return value |
There was a problem hiding this comment.
It would be better to use the value. Alternatively, there should be an explanation in the comment why it's safe to ignore the return value in this particular case.
There was a problem hiding this comment.
I found a use for the return value and added code to check whether input was successfully read from the terminal.
src/menu.cpp
Outdated
| { | ||
| print_menu(mystuff); | ||
| std::cout << "Change setting number: "; | ||
| fgets(choice_string, 9, stdin); // std:cin does not allow empty input | ||
| char* temp = fgets(choice_string, 9, stdin); // std:cin does not allow empty input | ||
| (void)temp; // avoid warning about unused return value |
There was a problem hiding this comment.
I would prefer that we fix warnings in a separate PR. It needs to be done very carefully.
There was a problem hiding this comment.
I actually found a use for the return value and added some lines to check whether input was successfully read.
| if (sscanf(&(buf[strlen(name) + 1]), "%"PRIu64, value) == 1) { | ||
| #else | ||
| if (sscanf(&(buf[strlen(name) + 1]), "%llu", value) == 1) { | ||
| #endif |
There was a problem hiding this comment.
I don't think this fix should be specific to MinGW. PRIu64 is a C99 feature. If we can standardize on C99, let's use it unconditionally. If we cannot, we need a proper fallback for older code that doesn't rely on long long being uint64_t.
There was a problem hiding this comment.
I did some testing and found that PRIu64 works on Windows and macOS without issues. However, compiling on Linux gives this message:
read_config.c:88:50: warning: format ‘%lu’ expects argument of type ‘long unsigned int *’, but argument 3 has type ‘long long unsigned int *’ [-Wformat=]
88 | if (sscanf(&(buf[strlen(name) + 1]), "%"PRIu64, value) == 1) {
ixfd64
force-pushed
the
0.16_build
branch
2 times, most recently
from
bdb41eb to
85ce4f2
Compare
on Linux