Makefile: avoid SSE42/AVX2 in non-release builds

[tamird]

This allows binaries built from source to run on older machines.

Closes #14443.

---

This change is 

[petermattis]

LGTM

[petermattis]

Why did you move this from start_jemalloc.go? That file is already protected by !stdmalloc which is being specified for windows builds.

[petermattis]

Please keep the // extern "C" comment.

[petermattis]

Perhaps we can also add some code which errors very early in the lifetime of the process if sse4.2 isn't available when we've compiled for it. RocksDB has the following code:

static bool isSSE42() {
#if defined(__GNUC__) && defined(__x86_64__) && !defined(IOS_CROSS_COMPILE)
  uint32_t c_;
  uint32_t d_;
  __asm__("cpuid" : "=c"(c_), "=d"(d_) : "a"(1) : "ebx");
  return c_ & (1U << 20);  // copied from CpuId.h in Folly.
#else
  return false;
#endif
}

[tamird]

I don't really understand this suggestion - if RocksDB has this code (and has since 2014: facebook/rocksdb@318eace), (1) why is USE_SSE a thing? and (2) why do we see crashes on machines that don't support SSE?

[tamird]

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/cli/jemalloc_unix.go, line 26 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Why did you move this from start_jemalloc.go? That file is already protected by !stdmalloc which is being specified for windows builds.

Because I'd rather reduce the amount of tribal knowledge required to compile on windows.

---

pkg/storage/engine/db.cc, line 42 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Please keep the // extern "C" comment.

Done, and added this comment to the equivalent place in CCL.

---

Comments from Reviewable

[petermattis]

I don't really understand this suggestion - if RocksDB has this code (and has since 2014: facebook/rocksdb@318eace), (1) why is USE_SSE a thing? and (2) why do we see crashes on machines that don't support SSE?

Probably because the -msse4.2 flag allows gcc/clang to use sse4.2 instructions. The suggestion is to check whether the processor supports sse4.2 before we run any cgo code.

---

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

pkg/cli/jemalloc_unix.go, line 26 at r5 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Because I'd rather reduce the amount of tribal knowledge required to compile on windows.

I'm not sure how it does that. Seems like random code movement to me.

---

Comments from Reviewable

[tamird]

Thanks. I'm going to investigate upstream a little bit.

---

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

pkg/cli/jemalloc_unix.go, line 26 at r5 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm not sure how it does that. Seems like random code movement to me.

It means that windows builds do not require stdmalloc to avoid linker errors.

---

Comments from Reviewable

[bdarnell]

USE_SSE is a thing because crc32c.cc needs to know whether nmmintrin.h is available. The ideal solution (from a compatibility perspective; I don't know if this would have a material performance impact) would be to make that header available without specifying -msse4.2 so that crc32c.cc can use the optimized version (when the runtime check passes) while still using conservative code generation for the rest of the binary.

---

Reviewed 5 of 5 files at r1, 2 of 2 files at r7, 1 of 4 files at r8, 6 of 6 files at r9.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Makefile, line 95 at r9 (raw file):

override LINKFLAGS += -X github.com/cockroachdb/cockroach/pkg/build.typ=development
else ifeq ($(TYPE),release)
override MFLAG = -msse -msse4.2

This will only work on amd64 architectures so it should probably have some sort of guard. (not a big deal yet, though, since we only test/support amd64)

What if a user wants to specify their own -m flag? It looks like the ones specified in this file will always take precedence, which is not ideal.

---

pkg/ccl/storageccl/engineccl/rocksdb_unix.go, line 9 at r9 (raw file):

//     https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/LICENSE

package engineccl

Needs a !windows build tag, right?

---

Comments from Reviewable

[tamird]

Review status: 2 of 17 files reviewed at latest revision, 4 unresolved discussions.

---

Makefile, line 95 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This will only work on amd64 architectures so it should probably have some sort of guard. (not a big deal yet, though, since we only test/support amd64)

What if a user wants to specify their own -m flag? It looks like the ones specified in this file will always take precedence, which is not ideal.

Done.

---

pkg/ccl/storageccl/engineccl/rocksdb_unix.go, line 9 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Needs a !windows build tag, right?

Thanks for spotting it. Done.

---

Comments from Reviewable

[bdarnell]

---

Review status: 2 of 17 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[petermattis]

---

Review status: 2 of 17 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[tamird]

Updated after #14840, PTAL.

[benesch]

We should offer some way for users building from source to opt-in to optimizations, but this is better than the current broken state of the world.

---

Reviewed 14 of 14 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[tamird]

Yeah, I think I'll repurpose TYPE=release for that once I'm able to resolve this TODO.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from Reviewable