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

Some minor fixes and cleanups #212

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

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 and others added 2 commits March 21, 2025 08:17
Co-authored-by: Mikael Simberg <mikael.simberg@iki.fi>
@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).

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