apache-arrow: set optimization to O2

[jonkeane]

• Have you followed the guidelines for contributing?
• Have you ensured that your commits follow the commit style guide?
• Have you checked that there aren't other open pull requests for the same formula update/change?
• Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
• Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
• Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

---

This is the workaround proposed in #94724 and I've confirmed that it works with apache-arrow's (extended) CI at apache/arrow#12364 (comment)

[carlocab]

Thanks!

[BrewTestBot]

🤖 A scheduled task has triggered a merge.

[pitrou]

If this fixes the issue of emitting an unsupported instruction, this is purely by chance, right? -Os does not enable any new instructions in itself. It just changes the heuristics used for choosing a particular instruction over another one. In other words, changing the optimization level only works by coincidence for this particular crash, but it doesn't fix the underlying issue which has to do with BMI2 being enabled at compile time.

[carlocab]

That's true -- that's why this is a workaround -- but it's not clear why BMI2 is being enabled at compile time. The most likely explanations to me are that there is either a bug in the Arrow build system, or there is a bug in the toolchain.

[pitrou]

We now have a working reproducer on CI without Homebrew involved. And it seems quite likely that it's a compiler bug: clang emits a BMI2 instruction even though only -msse4.2 is given on the command line.

The relevant log line is this one and there's nothing there that should enable BMI2:
https://github.com/ursacomputing/crossbow/runs/5189845009?check_suite_focus=true#step:8:1692

[carlocab]

Interesting. Have you worked out which versions of Clang are affected? Does it affect GCC? (It probably doesn't.)

[pitrou]

I don't have a Mac myself, I only have access to CI which is a bit painful for experimentation. So for now the affected version would be "AppleClang 13.0.0.13000029" :-)

[carlocab]

Does this not reproduce with Clang on Linux? Interesting.

[pitrou]

Hmm, I will try on Linux with the versions of clang I have here.

[pitrou]

I was not able to reproduce with either clang 10.0.0-4ubuntu1 or 12.0.0-3ubuntu1~20.04.4, with only -msse4.2.
Those clang versions do emit a shlx if I add -march=haswell, which is normal (Haswell CPUs support BMI2).

Perhaps Apple's version of clang carries some unofficial patches or is branched off a development version of clang?

[carlocab]

AppleClang 13.0.0.13000029 should correspond to LLVM/Clang 12.0.0. https://en.wikipedia.org/wiki/Xcode#Xcode_11.x_-_13.x_(since_SwiftUI_framework)_2

You could try a newer Clang (13) in CI by setting

CC="$(brew --prefix llvm)/bin/clang"
CXX="$(brew --prefix llvm)/bin/clang++"

[pitrou]

(also note I didn't find any matching bug report on the LLVM bug trackers)

[pitrou]

I tried to pick up a newer CLang as suggested by @carlocab above, but it doesn't seem to have succeeded. @jonkeane Can you help me?

[carlocab]

Hm. Weird. Can you try passing -DCMAKE_C_COMPILER=$(brew --prefix llvm)/bin/clang -DCMAKE_CXX_COMPILER=$(brew --prefix llvm)/bin/clang++ to cmake?

[pitrou]

Not really, because CMake here is hidden behind some non-trivial R build scripts, hence my question to @jonkeane .

[carlocab]

You can probably add it here: https://github.com/ursacomputing/crossbow/blob/0f75c8c6df77cb7316f99bd716e9784921868aeb/.github/workflows/crossbow.yml#L35

You'll need to interpolate brew --prefix llvm for /usr/local/opt/llvm manually.

[jonkeane]

Ok, I've added the necessary bits to ensure that R is using the newer clang (it wraps or overrides those env vars, and there's a slightly different way to specify them).

I'm pretty sure that this is picking up the right (newer) version of clang:

-- The C compiler identification is Clang 13.0.1
-- The CXX compiler identification is Clang 13.0.1

https://github.com/ursacomputing/crossbow/runs/5201239603?check_suite_focus=true#step:8:83

Though we still get the segfault: https://github.com/ursacomputing/crossbow/runs/5201239603?check_suite_focus=true#step:10:8126

[pitrou]

Well, if someone wants to experiment more and has an appropriately-configured macOS machine, they can try to reproduce some of the command line here: https://github.com/ursacomputing/crossbow/runs/5201239603?check_suite_focus=true#step:8:1691

and see whether the generated level_conversion.cc.o contains a BMI2 instruction, for example with:

objdump --demangle --disassemble level_conversion.cc.o | grep shl
# `shlx` is BMI2, `shl` and `shlq` are not

Myself, I cannot go any further.

[carlocab]

@jonkeane can you try this with gcc-11 and g++-11? Those should also be installed on the GitHub runner.