fix: make Dragonfly compatible with older systems #1755
Conversation
| @@ -8,11 +8,16 @@ HELIO_OPENSSL_USE_STATIC_LIBS = ON | |||
| HELIO_ENABLE_GIT_VERSION = ON | |||
| HELIO_WITH_UNWIND = OFF | |||
|
|
|||
| # equivalent to: if $(uname_m) == x86_64 || $(uname_m) == amd64 | |||
| ifneq (, $(filter $(BUILD_ARCH),x86_64 amd64)) | |||
| HELIO_MARCH_OPT := -march=core2 -msse4.1 -mpopcnt -mtune=skylake | |||
There was a problem hiding this comment.
I think skylake is a mistake here. They are running cpu family 6 model 13 which according to this is a pentium m. So you are tuning for an architecture that came later. Furthermore, as per the comments, shouldn't it be -march=native?
What am I missing here?
There was a problem hiding this comment.
Our responsibility as project maintainers to do what's right and not what was asked in the comments :)
We lowered the bar but we are not going to support 32bit processor pentium-m discontinued in 2009.
march=native will produce a code based on Azure github runner CPU. That's totally random and will be incompatible with many machines that run Dragonfly today. I will update the commit description
There was a problem hiding this comment.
+1 I got confused because you wrote Possibly addresses and the changes introduced in this PR should be incompatible with what is discussed on that thread.
There was a problem hiding this comment.
Yes, fixed the description.
| @@ -8,11 +8,16 @@ HELIO_OPENSSL_USE_STATIC_LIBS = ON | |||
| HELIO_ENABLE_GIT_VERSION = ON | |||
| HELIO_WITH_UNWIND = OFF | |||
|
|
|||
| # equivalent to: if $(uname_m) == x86_64 || $(uname_m) == amd64 | |||
| ifneq (, $(filter $(BUILD_ARCH),x86_64 amd64)) | |||
There was a problem hiding this comment.
Also isn't this only for the architecture and for the processor family ? I would expect something here that looks the microprocessor architecture and its familly
There was a problem hiding this comment.
no, we support x86_64 and arm64 architectures and we release binaries that can run for both architectures.
See here: https://github.com/dragonflydb/dragonfly/releases
There was a problem hiding this comment.
I know which architectures we support, it appeared to me that this PR was meant to address the microarchitecture mentioned on that PR. I got confused, that's all
|
also one more question, I never noticed this |
it's been used by releash.sh script but anyone can use it actually. One of our contributors wanted to provide a standard way to buid a project and I did not mind. |
Change the Makefile to configure the compile to produce code compatible with core2 architecture for x86_64 systems. Plus specify precise CPU extensions that we use in the code. Partly addresses #1519 Before the change we relied on default helio logic that configured the build to run on sandybridge for x86_64. This PR overrides x86_64 settings to much older core2 processor. Also, we remove an unneeded cmake include. Signed-off-by: Roman Gershman <[email protected]>
|
🚢 🇮🇹 |
romange
force-pushed
the
ReduceArchBuild
branch
from
47a5fbd to
12dc9e8
Compare
Change the Makefile to configure the compile to produce code
compatible with core2 architecture for x86_64 systems.
Plus specify precise CPU extensions that we use in the code.
Partly addresses #1519
Before the change we relied on default helio logic that configured
the build to run on sandybridge for x86_64.
This PR overrides x86_64 settings to much older core2 processor.
Also, we remove an unneeded cmake include.