Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openblas: bump with patch #6094

Merged
merged 2 commits into from
Jul 27, 2021
Merged

openblas: bump with patch #6094

merged 2 commits into from
Jul 27, 2021

Conversation

wattoc
Copy link
Contributor

@wattoc wattoc commented Jul 24, 2021

Run full build successfully, also launches digiKam, etc. without crashing.

I've also re-enabled AVX since this should be supported now, original issue for disabling it was here: OpenMathLib/OpenBLAS#1916 - this was due to Haiku not supporting AVX at all at the time.

It should be detecting the processor and using AVX if the processor supports it at runtime (basically compiles a bunch of different versions for various x64 cpu types.) If anyone is concerned about this it can always be taken out again with some performance hit, although if we can find someone with a Prescott x64 CPU, eg. a Pentium 4F and derivative to test, this would be helpful.

Patch should be upstreamed to openblas project so will hopefully not be needed in future. I can attempt a pull for this unless someone else wants to volunteer.

@korli korli merged commit 955233f into haikuports:master Jul 27, 2021
@brada4
Copy link

brada4 commented Jul 27, 2021

Please check on real AVX-2 and AVX-512 CPUs, they have bigger xsave file at least.

@wattoc
Copy link
Contributor Author

wattoc commented Jul 28, 2021

I only have access to Zen2 CPUs here (CPUs which support some sort of AVX anyway) - I've been testing with those. Not sure if there's anyone with Skylake X/Cannon Lake or newer to test AVX512 about.

@korli
Copy link
Contributor

korli commented Jul 28, 2021

@wattoc AVX-512 can be disabled, untested.

@brada4
Copy link

brada4 commented Jul 28, 2021

NO_AVX512=1 then

@wattoc
Copy link
Contributor Author

wattoc commented Jul 28, 2021

@korli this seems a bit extreme when you recently pushed out an update for this that was not working at all. I'm not sure why stringent testing requirements seem to only apply when I'm pushing packages here.

I would suggest leaving it enabled and wait for someone to report an issue.

@korli
Copy link
Contributor

korli commented Jul 28, 2021

@wattoc fine, not that I care much. I'm the one who implemented AVX support on Haiku x86_64, the reason why I argue.

@wattoc
Copy link
Contributor Author

wattoc commented Jul 28, 2021

If you want it disabled it is one line, it's easy enough to do - I would prefer evidence of it not working vs. speculation that it might not work. If I'd have done the AXV support I'd rather people have a chance to attempt to use it so I can be sure it works.

This package isn't really for my personal use, so is not based on some personal investment I'm just trying to help fix some bugs here - you're asking a lot of testing potentially (there are multiple kernels for various processor types) from someone doing this on a hobby basis who is naively trying to be helpful. It was literally broken for many people since 0.3.9 which went completely ignored for some time - so clearly has not been tested that well anyway. I'm not sure how I'm making it worse enabling an option that as far as I'm aware should work (I'm sure you can confirm that given you implemented AVX support) and if it doesn't is probably a great way to discover issues with the OS itself.

@brada4
Copy link

brada4 commented Jul 28, 2021

@wattoc Previous time there was no AVX in haiku kernel, I just suggested to check incrementally the new (register space) additions so that casual users do not return with "openblas crashes my gimp" sort of thing.

@pulkomandy
Copy link
Member

We have tested AVX support in Haiku in other places (for example the AVIF and Dav1d decoders use it if available). It's working fine. I don't know about AVX512 because no one with compatible hardware has tested Haiku on that, as far as I know.

The only thing to make sure is that openblas detects the instruction set before using it, and switches to another codepath if it isn't. But I understand that this is the case, so there should be no problem here.

@wattoc
Copy link
Contributor Author

wattoc commented Jul 28, 2021

@brada4 I appreciate what you and korli are saying - as far as I know korli added AVX support about a year ago, so surely that's enough time to have spotted some issues - we have some other things using it already as far as I'm aware. The main issue we have is lack of users with such hardware, so we need someone with a later Skylake X or similar to confirm AVX512 at some point, but that issue is not limited to OpenBLAS. If an issue occurs it's an easy place to toggle it off (we can ask the reporter to test a non-AVX512 target through the command line anyway) and then look at the issues on an OS level if need be. Since I now know a bit more about OpenBLAS I'm happy to try to assist if that happens.

I don't think it's a good approach to put artificial limits on things because we speculate there might be problems (although as far as anyone knows there shouldn't be) or we simply haven't tested that exact thing yet. It'd be like putting a 16GB limit on Haiku because nobody tested it with more than that and maybe something goes wrong.

Anyway we don't have GIMP yet :) The use case I know of here is something like digiKam (because that's how I became aware of the issue with someone raising a bug on it) using OpenCV and therefore OpenBLAS, which has been tested by myself as best I can with the resources I have. There are probably other things that potentially use OpenBLAS I haven't tested - but at some point you need to be reasonable. I committed a trivial bug fix that means we now count processors and don't create MAX_THREADS and enabled some options that SHOULD work.

@brada4
Copy link

brada4 commented Jul 28, 2021

@wattoc i back @korli , current XSAVE code in haiku supports only AVX2 (and SSE, and x87) , and no AVX-512

@wattoc
Copy link
Contributor Author

wattoc commented Jul 28, 2021

@brada4 I'll bring in a new pull request with NO_AVX512 defined in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants