search

cockroachdb / c-rocksdb

🚫 DEPRECATED
45 stars 19 forks source link

Enable sse4.2. #32

Closed petermattis closed 7 years ago

petermattis commented 7 years ago

Except on FreeBSD, which mirrors what build_detect_platform does.

See https://github.com/cockroachdb/cockroach/issues/12992

tamird commented 7 years ago

Does build_detect_platform do this? Is there anything else we're missing?

We should update the instructions in importh.sh so we don't have similar regressions in the future.

petermattis commented 7 years ago

Does build_detect_platform do this?

Yes.

Is there anything else we're missing?

I'm not sure. I took a look around before sending this PR and didn't see anything. I'll double-check.

We should update the instructions in importh.sh so we don't have similar regressions in the future.

This wasn't a regression. We've always been doing this wrong. Do you have a concrete suggestion for what update you'd like to see?

bdarnell commented 7 years ago

Does this mean that the resulting binary will require sse4.2, or is it still dynamically detected? (I see there's a dynamic lookup in crc32c.cc, but I don't know if other things will use sse4.2 unconditionally). It also looks like sse4.2 dates back to 2007 so it's probably safe to require it.

tamird commented 7 years ago

Something like https://github.com/cockroachdb/c-jemalloc/blob/master/import.sh#L10:L27

On Thu, Jan 19, 2017 at 10:36 AM, Peter Mattis notifications@github.com wrote:

Does build_detect_platform do this?

Yes.

Is there anything else we're missing?

I'm not sure. I took a look around before sending this PR and didn't see anything. I'll double-check.

We should update the instructions in importh.sh so we don't have similar regressions in the future.

This wasn't a regression. We've always been doing this wrong. Do you have a concrete suggestion for what update you'd like to see?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cockroachdb/c-rocksdb/pull/32#issuecomment-273808366, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdsPEtP5s4QwDS1RT3l5-fMhXKd5EN7ks5rT4MEgaJpZM4LoPaa .

petermattis commented 7 years ago

Does this mean that the resulting binary will require sse4.2, or is it still dynamically detected?

I'm not sure. I was looking at the gcc docs and couldn't tell. These flag settings are what build_detect_platform uses.

petermattis commented 7 years ago

@tamird Heh, import.sh already talks about running build_detect_platform and looking for flag changes. Unfortunately, you need to run USE_SSE=1 build_detect_platform. I'm still searching for when it is recommend to set that env var.

tamird commented 7 years ago

https://github.com/facebook/rocksdb/issues/1790

tamird commented 7 years ago

Sorry for the late comment, but looks like build_detect_platform only disables SSE on freebsd i386: https://github.com/facebook/rocksdb/blob/master/build_tools/build_detect_platform#L415:L418

petermattis commented 7 years ago

I don't have the ability to test the freebsd builds in any case. Perhaps @knz wants to take a look.

knz commented 7 years ago

I'd have enabled the sse thing on freebsd too, and just wait for someone to complain if it didn't work :-)