netlib-lapack: Permit package to build with clang+xlf compiler combination by djfitzgerald · Pull Request #8394 · spack/spack

djfitzgerald · 2018-06-05T21:53:15Z

Modified netlib-lapack to allow it to build using the llvm compiler suite with IBM xlf for the Fortran compiler. Related to #8311, #8388, #8389, #8391, #8392, #8393.

make it build with clang+xlf.

This change requires another change to the compilers/clang.py file to use the IBM XL Fortran compiler on CORAL systems.

davydden · 2018-06-05T21:58:51Z

var/spack/repos/builtin/packages/netlib-lapack/package.py


-    patch('ibm-xl.patch', when='@3.7: %xl')
-    patch('ibm-xl.patch', when='@3.7: %xl_r')
+    patch('ibm-xl.patch', when='@3.7:')


Does it break the build with GCC or Clang+gfortran?

@davydden This should not, as the patch applies to a portion of code that is only run when cmake detects that the Fortran compiler is an IBM XL or VisualAge compiler. I did verify that it did not break the build with gcc. However, the clang+gfortran build appears to be broken. It may already have been broken on Power, but I'll poke at it for a bit.

You can try building without this PR, and if’s broken, then most likely it was so already

@davydden I'm not sure why it was breaking before, but applying my patch over a clean Spack installation and its working now.

djfitzgerald · 2018-06-12T16:37:46Z

@adamjstewart @alalazo -- would either of you be willing to merge this PR?

adamjstewart · 2018-06-12T16:40:13Z

I don't really know anything about Fortran, Clang, or Xl, so this is outside my area of expertise...

davydden · 2018-06-12T16:45:19Z

var/spack/repos/builtin/packages/netlib-lapack/package.py

-                '-DCMAKE_Fortran_FLAGS=%s' % (
-                    ' '.join(self.spec.compiler_flags['fflags'])),
-            ])
+        if spec.satisfies('arch=linux-rhel7-ppc64le'):


Surely XL are not only used here? I think u need to rework this if.

Ps.i don’t know anything about XL either.

EDIT:

I would keep

if self.compiler.name == 'xl' or self.compiler.name == 'xl_r':

and add your part

elif spec.satisfies('arch=linux-rhel7-ppc64le'):

At least you would not break the setup for somebody who uses xl directly, not as a mixture with clang.

djfitzgerald · 2018-06-12T16:49:37Z

@davydden Could you suggest anyone that knows about Fortran and who might be a good candidate for a reviewer?

davydden · 2018-06-12T17:00:07Z

Could you suggest anyone that knows about Fortran and who might be a good candidate for a reviewer?

maybe try asking in Slack channel ? @tgamblin merged in the past some PR related to XL compilers (at least judging by history ).

On the second look, i think changes might break XL setup for other users

djfitzgerald · 2018-06-12T18:40:24Z

@davydden Please re-review. I addressed the changes that you had requested before (although now appear to have dismissed), although this new code won't have an effect until #8389 is merged. This change makes it so that xlf handling is done under clang only under the conditions where clang would be using xlf (that is to say, on Power64 Linux and when spack_f77 is "xlf_r").

davydden

LGTM modulo minor comments

davydden · 2018-06-12T18:48:41Z

var/spack/repos/builtin/packages/netlib-lapack/package.py

            cmake_args.extend(['-DCBLAS=OFF'])
            cmake_args.extend(['-DLAPACKE:BOOL=OFF'])
-
+        


please remove white space, falke8 will complain

davydden · 2018-06-12T18:49:31Z

var/spack/repos/builtin/packages/netlib-lapack/package.py

                    ' '.join(self.spec.compiler_flags['fflags'])),
            ])
+
+        if spec.satisfies('arch=linux-rhel7-ppc64le') and self.compiler.name == 'clang':


for clarity, please make it elif. I would move compiler.name part first so it fits better the previous if.

I feel that elif is inappropriate here (none of the other compiler checks around this line are in elif cases), but will make the other changes.

I feel that elif is inappropriate here (none of the other compiler checks around this line are in elif cases)

that's ok with me, that was just a suggestion.

davydden · 2018-06-12T18:54:23Z

var/spack/repos/builtin/packages/netlib-lapack/ibm-xl.patch

-+++ b/CBLAS/CMakeLists.txt
-@@ -12,8 +12,8 @@
-                          SYMBOL_NAMESPACE "F77_")
- if(NOT FortranCInterface_GLOBAL_FOUND OR NOT FortranCInterface_MODULE_FOUND)


btw, any reason to split this part of the patch into a separate cblas-cmake-fix.patch? The stand-alone part looks identical to this one. In other words, if you don't touch patches at all and only change the condition

patch('ibm-xl.patch', when='@3.7:')

you should get the same end result, right?

We wanted to explicitly move out the portion of the patch that could apply in more than just the IBM XL case.

ok, if you envision this to be useful later on. Just from this PR alone that is not obvious. Thus my comment.

djfitzgerald · 2018-06-12T19:37:48Z

@davydden Ready for re-review

djfitzgerald · 2018-06-12T19:40:49Z

@adamjstewart @alalazo @tgamblin -- would any of you be willing to merge this PR?

tgamblin · 2018-06-15T03:53:04Z

@djfitzgerald: looks like flake8 checks are failing here.

djfitzgerald · 2018-06-15T22:12:31Z

@tgamblin Hopefully my latest updates fix those flake8 failures. Checks are running now.

…g/netlib-lapack-2

davydden · 2018-06-16T05:55:22Z

@djfitzgerald

var/spack/repos/builtin/packages/netlib-lapack/package.py:162: [E125] continuation line with same indent as next logical line

you can also run checks locally: spack flake8.

djfitzgerald · 2018-06-18T15:04:30Z

@davydden Thanks for the tip. It should be all fixed now.

alalazo · 2019-02-12T14:38:54Z

@djfitzgerald Care to rebase? @tgamblin @davydden Any reason why this was not merged?

djfitzgerald · 2019-02-26T18:57:38Z

@alalazo I can take a stab at it, but it will have to wait until early March. I also haven't touched this stuff in almost 9 months, so I'll need to remember what the heck it was I was actually doing.

alalazo · 2019-11-18T11:25:02Z

Duplicate of #8311

alalazo · 2019-11-18T11:25:13Z

Closing as a duplicate

serbanmaerean added 2 commits June 5, 2018 17:48

Changed handling of XL and clang compilers for the PPC64LE platform to

d53c37c

make it build with clang+xlf.

Use spack_f77 instead of the Fortran compiler directly.

30324f6

This change requires another change to the compilers/clang.py file to use the IBM XL Fortran compiler on CORAL systems.

davydden reviewed Jun 5, 2018

View reviewed changes

davydden previously approved these changes Jun 6, 2018

View reviewed changes

djfitzgerald mentioned this pull request Jun 12, 2018

netlib-lapack: changed handling of XL and clang compilers for the PPC64LE. #8311

Closed

davydden reviewed Jun 12, 2018

View reviewed changes

Address code review comments

4f87207

davydden approved these changes Jun 12, 2018

View reviewed changes

davydden reviewed Jun 12, 2018

View reviewed changes

Address additional code review comments

50894f4

davydden approved these changes Jun 12, 2018

View reviewed changes

tgamblin approved these changes Jun 15, 2018

View reviewed changes

djfitzgerald added 2 commits June 15, 2018 17:44

Fix flake8 test failures

d3abc65

Fix flake8 test failures (fix bug in previous commit)

8f82ba0

djfitzgerald added 2 commits June 15, 2018 19:04

Fix additional flake8 error

0a0ddbb

Merge branch 'develop' of [email protected]:llnl/spack into fix-ibm-clan…

4c044c3

…g/netlib-lapack-2

djfitzgerald added 2 commits June 18, 2018 10:11

Fix additional flake8 failures

6826bdb

Finish fixing flake8 failures

361fd90

At newline at end of netlib-lapack/package.py to satisfy flake8

a3acb33

alalazo marked this as a duplicate of #8311 Nov 18, 2019

alalazo closed this Nov 18, 2019

		cmake_args.extend(['-DCBLAS=OFF'])
		cmake_args.extend(['-DLAPACKE:BOOL=OFF'])

Conversation

djfitzgerald commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djfitzgerald Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djfitzgerald commented Jun 12, 2018

Uh oh!

adamjstewart commented Jun 12, 2018

Uh oh!

davydden Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djfitzgerald commented Jun 12, 2018

Uh oh!

davydden commented Jun 12, 2018

Uh oh!

djfitzgerald commented Jun 12, 2018

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djfitzgerald commented Jun 12, 2018

Uh oh!

djfitzgerald commented Jun 12, 2018

Uh oh!

tgamblin commented Jun 15, 2018

Uh oh!

djfitzgerald commented Jun 15, 2018

Uh oh!

davydden commented Jun 16, 2018

Uh oh!

djfitzgerald commented Jun 18, 2018

Uh oh!

alalazo commented Feb 12, 2019

Uh oh!

djfitzgerald commented Feb 26, 2019

Uh oh!

alalazo commented Nov 18, 2019

Uh oh!

alalazo commented Nov 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

djfitzgerald commented Jun 5, 2018 •

edited

Loading

djfitzgerald Jun 6, 2018 •

edited

Loading

davydden Jun 12, 2018 •

edited

Loading