embree: refactor to work in Linux #74332
Conversation
danielnachun
added
the
linux to homebrew-core
| max_isa = "SSE4.2" | ||
|
|
||
| on_macos do | ||
| max_isa = "SSE2" unless MacOS.version.requires_sse42? | ||
| end |
There was a problem hiding this comment.
I think we require Nehalem as a minimal CPU, which would make this useless. Someone from @Homebrew/brew probably knows that better.
There was a problem hiding this comment.
There was a problem hiding this comment.
So in core the minimal supported CPU will always support sse4.2
There was a problem hiding this comment.
It won't work if you're still on High Sierra, though. I know we don't support High Sierra, but I also think we shouldn't knowingly break things for users still on it, especially when the accommodation is minor.
There was a problem hiding this comment.
I agree that this is a sufficiently small accommodation that it probably doesn't hurt to leave it in.
| @@ -17,7 +17,11 @@ class Embree < Formula | |||
| depends_on "tbb" | |||
|
|
|||
| def install | |||
| max_isa = MacOS.version.requires_sse42? ? "SSE4.2" : "SSE2" | |||
| max_isa = "SSE4.2" | |||
There was a problem hiding this comment.
On Linux oldest_cpu is core2 which does not support SSE4.2, but does support SSE3.
‘core2’ Intel Core 2 CPU with 64-bit extensions, MMX, SSE, SSE2, SSE3 and SSSE3 instruction set support.
‘nehalem’ Intel Nehalem CPU with 64-bit extensions, MMX, SSE, SSE2, SSE3, SSSE3, SSE4.1, SSE4.2 and POPCNT instruction set support.
See https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
and https://en.wikipedia.org/wiki/List_of_Intel_CPU_microarchitectures
There was a problem hiding this comment.
I see this isn't bottled on Linux, and will probably have cellar :any when it is bottled, so it will probably never be built from source unless explicitly requested. If our CI env supports SSE4.2 but a user tries to install the bottle on a system which only support SSE2, would that result in a broken bottle?
Perhaps the solution here is to use SSE2 on Linux so that the bottle works everywhere, and users who want better optimized instructions can build from source? This is the first time I've seen this particular scenario so I'm not sure how we should handle it.
There was a problem hiding this comment.
If our CI env supports SSE4.2 but a user tries to install the bottle on a system which only support SSE2, would that result in a broken bottle?
It depends on whether the executable detects at run time which instruction set is available, or if it always uses the instruction set that was specified at compile time. There's unfortunately no way to know without testing, or reading the upstream documentation.
There was a problem hiding this comment.
Perhaps the solution here is to use
SSE2on Linux so that the bottle works everywhere
Yes.
and users who want better optimized instructions can build from source?
We don't support building from source so I'd rather we didn't point them in that direction.
There was a problem hiding this comment.
On a similar topic, with GitHub Packages it would be possible to upload bottles compiled for different Intel CPUs (different microarchitectures) thanks to OCI/Docker's support for multi-architecture images.
There was a problem hiding this comment.
If it does runtime ISA checks, then can we drop max_isa from this formula?
There was a problem hiding this comment.
That's what I'm thinking. I need to test this on a server I have access to which has multiple generations of CPUs where I can try to build it with full set of supported ISAs (up to AVX-512) and then test on some older CPUs that definitely don't have that ISA. I'm not sure if there is anything on macOS which limits ISA support beyond the hardware itself - I don't know what determines MacOS.version.requires_sse42?.
There was a problem hiding this comment.
#requires_sse42? is just a check for whether the oldest CPU that runs a given OS is older than Nehalem, I think: https://rubydoc.brew.sh/OS/Mac/Version.html#requires_nehalem_cpu%3F-instance_method
Though now that you mention OpenBLAS I'm wondering whether this is needed:
homebrew-core/Formula/openblas.rb
Line 36 in 99f4b05
There was a problem hiding this comment.
That was probably related to some issues upstream had with the AVX-512 kernels not working correctly: OpenMathLib/OpenBLAS#2182. However, those issues seem to have been fixed, so we should see if it works properly again.
There was a problem hiding this comment.
Looks like you have access to a good testing environment for that :)
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Can we rebase this? What remains to be done to be able to move this forward? |
|
I rebased this. The unresolved question here is whether we even need to use a maximum supported ISA, as it seem this software checks if the needed instruction sets are supported at runtime. If that is the case, we can enable the newest ISA and for uses on older systems, it would automatically ignore the unsupported instructions without failing. I will try to this on the oldest server I can find in the cluster I use and confirm that it works with AVX-512 enabled. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
danielnachun
added
in progress
|
Going to close this until this is figured out. This PR has been open for way too long, and there is no point fighting with the stale bot forever :) |
brew install --build-from-source <formula>, where<formula>is the name of the formula you're submitting?brew test <formula>, where<formula>is the name of the formula you're submitting?brew audit --strict <formula>(after doingbrew install <formula>)?