Skip to content

Some minor fixes and cleanups #212

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

Merged
merged 8 commits into from
Mar 29, 2025

Conversation

albestro
Copy link
Contributor

@albestro albestro commented Feb 28, 2025

Changelog:

  • remove hard-coded variants (not used, templates ones are)
  • remove duplicated target from Makefile.compilers (70f1ebb)
  • added ruff to pyproject.toml
  • some formatting and typos fixing

About first point, I currently went for enabling gcc +profiled in the template as it was in the unused hard-coded variants, just because it sounds like a good thing. I'm not sure how much difference it will make, but it can make a difference in terms of cache reuse for future uenv builds. Indeed, AFAICT, spack info gcc reports

Variants:
...
    bootstrap [true]                   false, true
        Enable 3-stage bootstrap
...
    when %gcc+bootstrap
      profiled [false]                 false, true
          Use Profile Guided Optimization

which means that before this PR stackinator was configuring the bootstrapped compiler as gcc+bootstrap~profiled and now it should (implicitly) become gcc+bootstrap+profiled.

Waiting for your comments about this decision, which we can easily revert and keep the bootstrapped compiler as gcc+bootstrap~profiled.

according to section "Building with profile feedback" in https://gcc.gnu.org/install/build.html

> It is possible to use profile feedback to optimize the compiler itself.
> This should result in a faster compiler binary.
> Experiments done on x86 using gcc 3.3 showed approximately 7 percent speedup on compiling C programs.
@msimberg
Copy link
Contributor

FWIW, I think +profile seems sane to enable. It should rarely, if ever, result in a slower GCC, and regarding caches: yes, it'll invalidate the cache the first time, but we have the cache exactly to avoid potentially expensive builds for subsequent builds.

@msimberg
Copy link
Contributor

Is this a pre-cleanup PR to work towards removing the "stackinator-compiler-bootstrap"?

@albestro
Copy link
Contributor Author

Is this a pre-cleanup PR to work towards removing the "stackinator-compiler-bootstrap"?

Well, I'm not sure how to answer 😁 There are two open topics, namely "compiler bootstrapping" and "compilers as dependencies", that are somehow related from my point of view.

At the moment I'm looking more on the latter topic (for which I'm going to open a PR with some exploratory work I did), which somehow involves also the former one.

More generally, these are small things that I found out while I was studying different parts of the codebase, and I thought they were worth being fixed.

I think the answer might be: it is some cleanup work I found myself doing while I was studying the codebase (also) for that topic. But nothing strictly required or preparatory I would say.

@msimberg msimberg requested a review from bcumming March 21, 2025 08:25
@bcumming
Copy link
Member

Some context: we should "fork" stackinator soon - keeping the current master for supporting Spack v0.23, while we make any breaking changes required to support Spack v1.0 (and to make changes that take advantage of new features in v1.0).

I am actually a bit reluctant to invalidate the build caches and make a change to how compilers are built in the "old" branch. It works and it ain't broke, and all that.

Can we instead focus on making this change in the "new" version?

@albestro
Copy link
Contributor Author

After the meeting we had last week, IIUC we target for this PR just changes that do not alter the behaviour and now new functionalities (that will end up in the fork targeting spack 1.0).

With the last commit b16745e I reverted the change about +profiled, so keeping +bootstrap~profiled and re-use what it should be already there in the cache.

In order to do a quick summary check, I've run make compilers for:

  1. master
  2. PR before last commit (+profiled)
  3. PR current status (implicit ~profiled)

and I compared the store folder with the packages installed by spack doing

  • master (0) vs new (1)
  • master (0) vs new-change (2)
[daint][ialberto@daint-ln003 ialberto]$ diff test-master/store/linux-sles15-neoverse_v2/gcc-13.3.0/ test-new/store/linux-sles15-neoverse_v2/gcc-13.3.0 | grep -v "Common subdirectories" | sort
Only in test-master/store/linux-sles15-neoverse_v2/gcc-13.3.0/: gcc-13.3.0-jiirevuiuw4kt4m5dyx4hhq4q3yr366a
Only in test-new/store/linux-sles15-neoverse_v2/gcc-13.3.0: gcc-13.3.0-gfqt5r3i5apfkdnduidjdc4pvwzqbt53
[daint][ialberto@daint-ln003 ialberto]$ diff test-master/store/linux-sles15-neoverse_v2/gcc-13.3.0/ test-new-change/store/linux-sles15-neoverse_v2/gcc-13.3.0 | grep -v "Common subdirectories" | sort
[daint][ialberto@daint-ln003 ialberto]$

As it can be seen above, despite it is a trivial tests, at least it confirms that current status of the PR should end up doing exactly the same, while without the last commit we were getting a different gcc (because of the +profiled).

Copy link
Member

@bcumming bcumming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
Thanks Alberto!

@bcumming bcumming merged commit 855b56b into eth-cscs:master Mar 29, 2025
2 checks passed
msimberg added a commit that referenced this pull request Jun 24, 2025
Relevant changes are:
- drop `compilers.yaml` in favour of `packages.yaml`
- use variants to distinguish bootstrap compiler (aka `bootstrap`) from bootstrapped compiler (aka `gcc`);
- use `+profiled` for bootstrapped compiler (it was left out from #212)

## Notes
### Distinguishing compilers from various steps

Currently the tool build a bootstrapped gcc compiler explicitly via these steps:
1. `gcc@7` look for system compiler
2. `gcc@12 % gcc@7` build a bootstrap compiler using the system one
3. `gcc@12 % gcc@12` build a bootstrapped compiler using the bootstrapp one from previous step

Before `[email protected]` this was doable, because compilers were separate from packages. Indeed, we can simplify saying that `%` was looking into `compilers.yaml`. Unfortunately, this is not possible anymore since compilers have become packages, so `%` looks into `packages.yaml`.

Previously compilers information from one step to the next were passed via `compilers.yaml`, while now it is passed via `packages.yaml`. This turns out to be a problem when compilers in two steps are of the same version, e.g. step 2 and step3, where with `spack compiler find` we get something like 

```yaml
# packages.yaml
packages:
  gcc:
    external:
      spec: gcc@12
      ...
```

and when we do `spack concretize gcc@12` we get it concretized to that one already specified as external, no matter what `variants:` we ask for.

The current workaround (see 5a0f1e2) is to:
- make the external spec more specific (using `yq`)
- make the requested spec more specific (`variants` are not enough)
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