Fixed OpenBLAS compilation error on POWER8 with GCC 7.#4783
Fixed OpenBLAS compilation error on POWER8 with GCC 7.#4783adamjstewart merged 2 commits intospack:developfrom
Conversation
katherlee
changed the title
davydden
left a comment
davydden
left a comment
There was a problem hiding this comment.
Wow, that's a huge patch....
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
Can you at least restrict patching only to the cases that were giving a failure?
|
|
||
| # Fixes compilation error on POWER8 with GCC 7 | ||
| # https://github.com/xianyi/OpenBLAS/pull/1098 | ||
| patch('power8.patch', when='@0.2.18:0.2.19') |
There was a problem hiding this comment.
Just my opinion: for a patch as big as this one I think we should consider adding a conflict on ppc64 (or whatever is the right platform) instead of patching the library. This is just to maintain the claim that we are working on pristine sources.
In any case (i.e. if people disagree with the above), I think patching should be done only when we are building on power8 and with gcc@7:
patch('power8.patch', when='@0.2.18:0.2.19 %[email protected]: target=ppc64')There was a problem hiding this comment.
I think it is necessary to add a conflict for older versions which don't have PowerPC support. According to OpenMathLib/OpenBLAS#1078 the asm code before patching was simply broken, so I would prefer patching affected versions whenever building on a PowerPC. The patch was created from an merged official PR so I would say it doesn't conflict with the claim of yours.
There was a problem hiding this comment.
The patch was created from an merged official PR so I would say it doesn't conflict with the claim of yours.
This statement is kind of debatable. Think of this counter-example: I could prepare a patch that goes from 0.2.18 to 0.2.19 and apply it whenever you build 0.2.18. I don't think I can claim I am using [email protected] pristine sources at that point.
I understand that it is better to fix something rather than just saying it doesn't work, but 7k lines are really a lot. That's why I asked other people what they think about it. Imo the best solution would be:
- a new
openblasrelease from upstream - conflicts for
target=ppc64 %[email protected]:for older versions
but that's outside of our direct control. Another thing that could work is to just rely on openblas@develop.
There was a problem hiding this comment.
Make sense. And it seems the next official release is underway. Let's what other people think.
|
@katherlee I cc'ed @adamjstewart and @lee218llnl just to know what they think about this (as |
|
Hmm, I understand that this is a large patch, and I understand the desire to be working from clean source, but isn't this the classic case when a patch is required? This patch (shouldn't/isn't supposed to) change any behavior. It fixes a bug in compilation. Thus the patched source (should) behave identically. It's still the same release, with a bug fix in it. And since it has been merged into develop, that's a strong argument for its safety and validity. I guess we need to have a more general conversation here. Should we always encourage people to use |
|
I don't have a strong opinion here, but I tend to be pragmatic and like to fix things when they are broken (and only for the cases they are broken). I also have no problem with the size of the patch. Perhaps this would be best raised in a concall to establish a general best practices. All said, my vote for this PR is to do the patching, but only on PPC64 with gcc. |
alalazo
dismissed
their stale review
Fine with me. The patch has been restricted to ppc64 with gcc.
5 participants
Bug present in versions 0.2.18 and 0.2.19, and should be fixed in any future releases. Patch created from OpenMathLib/OpenBLAS#1098. Fixes #4782.