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

sysdetect: fix bug where Makefile sets -ffree-form everywhere #147

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

tgamblin
Copy link
Contributor

@tgamblin tgamblin commented Dec 27, 2023

To fix ARM compiler builds, #108 added these lines to the Makefile for sysdetect tests:

+ FFLAGS += -ffree-form
+
  ifeq ($(notdir $(F77)),gfortran)
      FFLAGS +=-ffree-form -ffree-line-length-none
  else ifeq ($(notdir $(F77)),flang)
      FFLAGS +=-ffree-form
  else ifneq ($(findstring $(notdir $(F77)), $(intel_compilers)),)
      FFLAGS +=-free
  else ifneq ($(findstring $(notdir $(F77)), $(cray_compilers)),)
      FFLAGS +=-ffree
  endif

but this adds -ffree-form to every compiler. This breaks cray compiler builds for PAPI 7.1.0:

     1307    /home/gitlab-runner-protected-3/builds/Cj9btwqn/0/spack/spack/lib/
             spack/env/cce/ftn -ffree-form -ffree -I../../.. -o query_device_si
             mple_f query_device_simple_f.F ../../../libpapi.a
     1308    ftn-78 ftn: ERROR in command line
     1309      The -f option has an invalid argument, "free-form".

You can see that CCE gets both -ffree-form and -ffree there.

This patch removes the global -ffree-form and makes the flang condition above a bit more permissive, so that both flang, armflang, amdflang, or really anything ending in flang adds -free-form:

       FFLAGS +=-ffree-form -ffree-line-length-none
-  else ifeq ($(notdir $(F77)),flang)
+  else ifeq ($(patsubst %flang,,$(notdir $(F77))),)  # compiler name ends with flang
       FFLAGS +=-ffree-form

This should fix the bug that #108 fixed and it should also work on other compilers.

Pull Request Description

Author Checklist

  • Description
    Why this PR exists. Reference all relevant information, including background, issues, test failures, etc
  • Commits
    Commits are self contained and only do one thing
    Commits have a header of the form: module: short description
    Commits have a body (whenever relevant) containing a detailed description of the addressed problem and its solution
  • Tests
    The PR needs to pass all the tests

To fix ARM compiler builds, icl-utk-edu#108 added these lines to the `Makefile` for `sysdetect`
tests:

```diff
+ FFLAGS += -ffree-form
+
  ifeq ($(notdir $(F77)),gfortran)
      FFLAGS +=-ffree-form -ffree-line-length-none
  else ifeq ($(notdir $(F77)),flang)
      FFLAGS +=-ffree-form
  else ifneq ($(findstring $(notdir $(F77)), $(intel_compilers)),)
      FFLAGS +=-free
  else ifneq ($(findstring $(notdir $(F77)), $(cray_compilers)),)
      FFLAGS +=-ffree
  endif
```

but this adds `-ffree-form` to every compiler. This breaks
[cray compiler builds for PAPI 7.1.0](https://gitlab.spack.io/spack/spack/-/jobs/9684882):

```
     1307    /home/gitlab-runner-protected-3/builds/Cj9btwqn/0/spack/spack/lib/
             spack/env/cce/ftn -ffree-form -ffree -I../../.. -o query_device_si
             mple_f query_device_simple_f.F ../../../libpapi.a
     1308    ftn-78 ftn: ERROR in command line
     1309      The -f option has an invalid argument, "free-form".
```

You can see that CCE gets both `-ffree-form` and `-ffree` there.

This patch removes the global `-ffree-form` and makes the flang condition above a bit
more permissive, so that both `flang`, `armflang`, `amdflang`, or really anything ending
in `flang` adds `-free-form`:

```diff
       FFLAGS +=-ffree-form -ffree-line-length-none
-  else ifeq ($(notdir $(F77)),flang)
+  else ifeq ($(patsubst %flang,,$(notdir $(F77))),)  # compiler name ends with flang
       FFLAGS +=-ffree-form
```

This should fix the bug that icl-utk-edu#108 fixed and it should also work on other compilers.
@tgamblin
Copy link
Contributor Author

tgamblin commented Dec 27, 2023

The pipeline on spack/spack#41886, confirms this fixes the build for CCE (successful build is here: https://gitlab.spack.io/spack/spack/-/jobs/9685218).

We don't currently test PAPI 7.1.0 with ARM compilers, so maybe @gcongiu can verify whether this fixes the issue he tried to solve in #108 -- I think it should.

@tgamblin
Copy link
Contributor Author

I'm not sure what to do with the CI failure here. papi_spack is failing because this patch is already in Spack, and the patch fails to apply :). We don't currently have a good way to notice that. We've tried before:

But unfortunately making patch application idempotent can cause us to silently fail to apply patches (unix patch does not give you a way to differentiate between the two cases) so we had to revert:

Any suggestions?

@gcongiu
Copy link
Contributor

gcongiu commented Dec 28, 2023

@tgamblin I would merge this PR anyway as the failure is not related to the patch (unless @jagode prefers otherwise). Maybe we could avoid this problem from happening again in the future by creating and testing PRs for the new PAPI versions in spack before releasing them? I am open to suggestions

@tgamblin
Copy link
Contributor Author

Actually, looking at your CI, think I can fix this on the spack side if I just restrict the patch to 7.1.0. You’re building papi@master in CI, and this patch will be in 7.1.1, right?

@gcongiu
Copy link
Contributor

gcongiu commented Dec 28, 2023

@tgamblin it will definitely be in the next release, but that will not necessarily be versioned as 7.1.1.

@tgamblin
Copy link
Contributor Author

I should ask better questions. If I restrict the patch to 7.1.0, does that cover all the PAPIs we might see with the issue?

@gcongiu
Copy link
Contributor

gcongiu commented Dec 28, 2023

@tgamblin if you restrict the patch to papi@7.1.0 I think that should do it. The sysdetect component was introduced with papi@7.0.0 and papi@7.0.1 was released soon after to fix some gcc related link time optimisation problem with more recent gcc versions (IIRC). However, none of the aforementioned versions ever worked with spack.

@tgamblin
Copy link
Contributor Author

Ok, spack/spack#41892 restricts the patch to 7.1.0 so CI should pass.

@G-Ragghianti
Copy link
Contributor

I've triggered a rerun of the papi-spack CI test.

@G-Ragghianti
Copy link
Contributor

It is fixed! A holiday miracle!

@gcongiu gcongiu merged commit 871e168 into icl-utk-edu:master Jan 2, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants