[Premerge] Add flang-rt #128678

Meinersbur · 2025-02-25T08:58:53Z

Flang's runtime can now be built using LLVM's LLVM_ENABLE_RUNTIMES mechanism, with the intent to remove the old mechanism in #124126. Update the pre-merge builders to use the new mechanism.

In the current form, #124126 actually will add LLVM_ENABLE_RUNTIMES=flang-rt implicitly, so no change is strictly needed. I still think it is a good idea to do it explicitly and in advance.

I also update the dependencies:

flang requires mlir (but not flang-rt, that would be a circular dependency)
flang-rt requires flang and its transitive dependencies

On Windows, flang-rt also requires compiler-rt, but which is not building on Windows anyway.

Also consider llvm/llvm-zorg#396

boomanaiden154 · 2025-03-03T18:00:09Z

.ci/compute-projects.sh

@@ -47,6 +48,7 @@ function compute-projects-to-test() {
      # Flang is not stable in Windows CI at the moment
      if [[ $isForWindows == 0 ]]; then
        echo flang
+        echo flang-rt


Putting it here will still build it using the LLVM_ENABLE_PROJECTS mechanism. Is that intentional?

No it is not. I think I assumed that it worked like zorg's depends_on_projects.

But now that I look closer to it, I am even more puzzled. If there is a file changed in a runtime, let's say libcxx, it will not be added to linux_projects_to_test, and hence nothing is added to linux_runtimes_to_test? Am I missing something? Why does all_projects list also the runtimes?

They're all built as projects currently. It's on my TODO list to migrate them, but I haven't gotten around to it yet.

As far as I understand libcxx;libcxxabi;libunwind are the three that go through the runtimes build.

If everything works like this, I'm perfectly fine with merging and then changing it around when I switch how the runtimes build works in the premerge system. Otherwise, having it be implicit added when building flang is fine with me, and we can revisit when I switch stuff over to the proper runtimes build.

I added flang-rt to compute-runtimes-to-test instead. Please also review the changes in add-dependencies, I think the entry on flang-rt is useless then.

Have you tested this locally? I don't think this will work as is. On Linux the runtimes explicitly built through the runtimes build are in

llvm-project/.ci/monolithic-linux.sh

Line 95 in 6eefadd

cmake -S "${MONOREPO_ROOT}/runtimes" -B "${RUNTIMES_BUILD_DIR}" -GNinja \

I would think the path to Flang would need to be passed in there? No idea how the plumbing works for flang-rt though. The runtimes are also built three separate times in different C++ configurations, which doesn't make sense for a library written in Fortran, although looking at the source, it seems like most of it is in C++?

If this isn't a necessary change for keeping the flang build working, it might be worth just waiting until I've had a chance to rework how we test runtimes in premerge.

You are correct, the path to Flang must be passed to the runtimes build. Thanks for noticing.

I have not tested it locally yet. There are some reasons for that

I was assuming that the premerge-monolithic-linux builder (which I tested, see LLVM_ENABLE_RUNTIMES=flang-rt for premerge-monolithic-* llvm-zorg#396) was resembling the premerge configuration (which is a bootstrapping build), as was alluded to by a comment.

I don't have a local BuildKite infrastructure

I was hoping that the pre-merge check of this PR was testing it. While it was in draft mode, it did not show errors.

The configuration makes use of a Google-proprietary extensions, such as "-gmlt"

I will try to make it run locally anyway.

…time_premerge

Meinersbur · 2025-03-05T15:57:25Z

I verified it locally. I also removed changes that I think are not strictly needed. So now whenever flang is built, flang-rt is built as well. CMAKE_Fortran_COMPILER is added to point to the Fortran compiler in the runtimes built. If flang is not being built, it is ignored by CMake.

I also added a small change to the flang/ directory to trigger a pre-merge built of flang, to actually test it.

Some Background: In its current form #124126 will implicitly add LLVM_ENABLE_RUNTIMES=flang-rt (unless specifying FLANG_ENABLE_FLANG_RT=OFF that is). That is, the pre-merge will build flang-rt anyway at that point, but using the bootstrapping build and without check-flang-rt. For the pre-merge build this means that we get an ugly mix of bootstrapping-build of flang-rt and a runtimes-standalone build of libcxx,libcxxabi,libunwind at the same time. I am proactively adapting the buildbots to not have unfortunate surprises when #124126 is pushed, having to handle all at once / repeated reverts.

If you think that would be the lesser evil, I would be OK with skipping this PR.

boomanaiden154

Have you tested this running premerge on flang-rt change? I think that would still go through the normal LLVM_ENABLE_PROJECTS build which might not be what you want.

If not testing changes solely to flang-rt is fine, then adding it to the exclude lists should be fine. If it needs to be tested in the runtimes build, adding it to the exclude list and making sure it propagates through compute-runtimes-to-test I think would work (after making sure flang is added as a dependency).

.ci/monolithic-local.sh

Update premerge-monolithic-windows and premerge-monolithic-linux to prepare for the removal of the "projects" build of the flang runtime in llvm/llvm-project#124126. This does not change the actual premerge GitHub action, which is done by llvm/llvm-project#128678 For premerge-monolithic-linux, add flang-rt to the LLVM_ENABLE_RUNTIMES list, indirectly by adding it to `depends_on_projects` which also updated the build scheduler. For premerge-monolithic-windows, remove building flang to match the actual pre-merge build which disabled building flang on Windows in llvm/llvm-project@e4b424a. Adding flang-rt would also require compiler-rt (which was always required on Windows, but now there is a regression test for it) and check-compiler-rt is currently failing. Split off from #333. Verified to work locally using instruction from https://llvm.org/docs/HowToAddABuilder.html#testing-a-builder-config-locally. With the exception of `-gmlt` which seems to be a Google-only extention of Clang. Affected builders: * [premerge-monolithic-windows](https://lab.llvm.org/buildbot/#/builders/35) * [premerge-monolithic-linux](https://lab.llvm.org/buildbot/#/builders/153) Affected workers: * [premerge-windows-1](https://lab.llvm.org/buildbot/#/workers/153) * [premerge-linux-1](https://lab.llvm.org/buildbot/#/workers/110)

Meinersbur · 2025-03-06T15:25:00Z

Have you tested this running premerge on flang-rt change? I think that would still go through the normal LLVM_ENABLE_PROJECTS build which might not be what you want.

If not testing changes solely to flang-rt is fine, then adding it to the exclude lists should be fine. If it needs to be tested in the runtimes build, adding it to the exclude list and making sure it propagates through compute-runtimes-to-test I think would work (after making sure flang is added as a dependency).

Thanks for noticing. It seems that that libcxx, etc. are already not built when they are changes (due to not being in all_projects), but have their own libcxx-ci trigger which I don't understand how it works. I first didn't intend to repair this, but seems it wasn't too difficult to add a workaround for flang-rt.

boomanaiden154

The build failiures on the windows buildkite look relevant.

Meinersbur · 2025-03-06T22:50:57Z

The build failiures on the windows buildkite look relevant.

Indeed. I should not trigger a build on Windows for something that is not even tested on Windows.

Btw, I also noticed that it adds LLVM_ENABLE_PROJECTS=llvm, even though llvm is not a valid argument for LLVM_ENABLE_PROJECTS. I left it in the result of compute-projects-to-test because I think it is what triggers check-llvm.

boomanaiden154

Mostly looks good to me at this point.

I'm assuming you've tested the latest version somewhere?

boomanaiden154 · 2025-03-11T01:17:01Z

.ci/compute-projects.sh

      # Flang is not stable in Windows CI at the moment
      if [[ $isForWindows == 0 ]]; then
        echo flang
      fi
    ;;
    *)
-      # Nothing to do
+      echo "${project}"


Is there a reason this moved?

At the old location, it would always output the project, including "flang-rt". I could have added an if-condition to not print flang-rt, or filter it out afterwards in like in exclude-linux/exclude-windows but that doesn't seem its purpose, or just let it add it to "LLVM_ENABLE_PROJECTS" even though it it ignored there (like "llvm"). I found the best solution is to change the function to let each case decide whether to prints itself, which would also e.g. allow to not print "llvm" at some point.

I tried to avoid modifying https://github.com/llvm/llvm-project/blob/main/.ci/generate-buildkite-pipeline-premerge itself, there are copies of that code, like in https://github.com/llvm/llvm-project/blob/main/.github/workflows/premerge.yaml, but I could not get around changing all_projects.

boomanaiden154 · 2025-03-11T01:17:48Z

.ci/compute-projects.sh

@@ -65,6 +73,11 @@ function compute-runtimes-to-test() {
        echo $p
      done
    ;;
+    flang)


If a patch touches flang-rt and flang, it seems like we might end up with some duplication given the cyclical dependency here?

Not the case. compute-runtimes-to-test receives the aguments to "LLVM_ENABLE_PROJECTS" and prints the arguments to "LLVM_ENABLE_RUNTIMES". There is no function for the reverse direction.

If you one day rewrite the mechanism, consider giving different names to "top-level directory in the repository", "valid argument to LLVM_ENABLE_PROJECTS", and "valid argument to LLVM_ENABLE_RUNTIMES". They are currently all called "projects" in some form.

Meinersbur · 2025-03-11T13:34:54Z

I'm assuming you've tested the latest version somewhere?

Using the pre-merge itself, which seems to be more accurate than my local testing: https://buildkite.com/llvm-project/github-pull-requests/builds/154307#01956d9d-f62e-4104-b9bd-ca2c5b551686

This only tests if there is a change in the flang-rt/ directory. If you think it is necessary to also test various combinations of tests in other directories, please let me know.

boomanaiden154 · 2025-03-11T21:08:36Z

This only tests if there is a change in the flang-rt/ directory. If you think it is necessary to also test various combinations of tests in other directories, please let me know.

If you're able to test that touching something in flang/ also works, I think that would be helpful.

Once we migrate fully over to a Github workflow I'm hoping to rewrite these scripts in Python and add some unittests which should make things a lot easier.

boomanaiden154

Seems fine to me for now given that we're planning on reworking the runtimes build in premerge soon.

Meinersbur · 2025-03-12T23:49:51Z

Tested with a flang/ change here: #130955. Interestingly, it builds flang even on Windows: https://buildkite.com/llvm-project/github-pull-requests/builds/156355#01958a65-0b87-4bbe-bc22-6eda010c2b94

Another test with a flang/ change without any change in .ci (#130960) reveals that it was always that way: https://buildkite.com/llvm-project/github-pull-requests/builds/156364#01958a85-e4f9-4be5-a217-dcfcbcfed301

Reason is what I already mentioned: compute-projects-to-test() always echo's the directory that has changed, with this PR it just falls back to the *) case. Still merge? I don't intend to fix all the legacy problems.

boomanaiden154 · 2025-03-13T00:38:45Z

Still merge? I don't intend to fix all the legacy problems.

Yeah, no need to fix legacy problems. We're hopefully going to rewrite this soon anyways.

Meinersbur · 2025-03-13T11:18:11Z

@boomanaiden154 Thank you

Flang's runtime can now be built using LLVM's LLVM_ENABLE_RUNTIMES mechanism, with the intent to remove the old mechanism in llvm#124126. Update the pre-merge builders to use the new mechanism. In the current form, llvm#124126 actually will add LLVM_ENABLE_RUNTIMES=flang-rt implicitly, so no change is strictly needed. I still think it is a good idea to do it explicitly and in advance. On Windows, flang-rt also requires compiler-rt, but which is not building on Windows anyway.

This reverts commit 95d28fe. I did not fully realize the implications of this change when reviewing. With how it is set up currently, it causes clang and all of the runtimes to be built and tested everytime a change to MLIR is made. This is a large regression in build/test time, which seems to have been causing large queueing delays. Reverting for now. Once we rework the runtimes build for premerge (which I hope to do soon, ideally in the next week), I will make sure flang-rt gets added in.

Meinersbur added 2 commits February 25, 2025 09:31

[Premerge] Add flang-rt

9ebeeae

Don't change deps

afada27

This was referenced Feb 28, 2025

[Flang] Remove FLANG_INCLUDE_RUNTIME #124126

Open

LLVM_ENABLE_RUNTIMES=flang-rt for premerge-monolithic-* llvm/llvm-zorg#396

Merged

flang-rt has dependencies

8345c19

Meinersbur marked this pull request as ready for review March 3, 2025 09:36

Meinersbur requested review from boomanaiden154 and metaflow March 3, 2025 15:33

boomanaiden154 removed the request for review from metaflow March 3, 2025 16:47

boomanaiden154 reviewed Mar 3, 2025

View reviewed changes

Meinersbur added 10 commits March 4, 2025 00:20

flang-rt is a runtime

da2a539

Merge branch 'flang_runtime_premerge' into users/meinersbur/flang_run…

93a7713

…time_premerge

Fix syntax error

a8a9323

flang_runtime_amdgpu-offload

a7b728c

Pass Flang as Fortran compiler

b0d0879

Merge branch 'main' into users/meinersbur/flang_runtime_premerge

8c41ae6

Minimal changes

e8b028a

Add change in flang/ to trigger pre-merge check

67809ce

Undo unrelated change

d2144a0

Set FLANG_ENABLE_FLANG_RT=OFF

26bb93a

Pass LLVM_BINARY_DIR required for testing

6e7d7fd

boomanaiden154 reviewed Mar 5, 2025

View reviewed changes

.ci/monolithic-local.sh Outdated Show resolved Hide resolved

Adjust all runtime build configurations

bed0c3f

Meinersbur added 4 commits March 6, 2025 15:58

Trigger build of Flang when flang-rt is changed

59ec1ee

Test change in flang-rt to trigger flang compilation

637f662

Consider changes in flang-rt

881b6bf

Exclude flang-rt from LLVM_ENABLE_PROJECTS

3d95f2e

boomanaiden154 reviewed Mar 6, 2025

View reviewed changes

Do not add flang-rt to projects-to-test in the first place

b2354ff

boomanaiden154 reviewed Mar 11, 2025

View reviewed changes

boomanaiden154 approved these changes Mar 11, 2025

View reviewed changes

This was referenced Mar 12, 2025

Users/meinersbur/flang runtime premerge flangtest #130955

Closed

[Flang][NFC] Fix typo #130960

Merged

Meinersbur merged commit 95d28fe into main Mar 13, 2025
10 checks passed

Meinersbur deleted the users/meinersbur/flang_runtime_premerge branch March 13, 2025 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Premerge] Add flang-rt #128678

[Premerge] Add flang-rt #128678

Meinersbur commented Feb 25, 2025 •

edited

Loading

boomanaiden154 Mar 3, 2025

Meinersbur Mar 3, 2025

boomanaiden154 Mar 3, 2025

Meinersbur Mar 3, 2025

boomanaiden154 Mar 5, 2025

Meinersbur Mar 5, 2025

Meinersbur commented Mar 5, 2025

boomanaiden154 left a comment

Meinersbur commented Mar 6, 2025

boomanaiden154 left a comment

Meinersbur commented Mar 6, 2025 •

edited

Loading

boomanaiden154 left a comment •

edited

Loading

boomanaiden154 Mar 11, 2025

Meinersbur Mar 11, 2025 •

edited

Loading

boomanaiden154 Mar 11, 2025

Meinersbur Mar 11, 2025

Meinersbur commented Mar 11, 2025

boomanaiden154 commented Mar 11, 2025

boomanaiden154 left a comment

Meinersbur commented Mar 12, 2025 •

edited

Loading

boomanaiden154 commented Mar 13, 2025

Meinersbur commented Mar 13, 2025

@@ @@ -65,6 +73,11 @@ function compute-runtimes-to-test() { @@
                       echo $p
                     done
                   ;;
+                  flang)

[Premerge] Add flang-rt #128678

[Premerge] Add flang-rt #128678

Conversation

Meinersbur commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meinersbur commented Mar 5, 2025

boomanaiden154 left a comment

Choose a reason for hiding this comment

Meinersbur commented Mar 6, 2025

boomanaiden154 left a comment

Choose a reason for hiding this comment

Meinersbur commented Mar 6, 2025 • edited Loading

boomanaiden154 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meinersbur Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meinersbur commented Mar 11, 2025

boomanaiden154 commented Mar 11, 2025

boomanaiden154 left a comment

Choose a reason for hiding this comment

Meinersbur commented Mar 12, 2025 • edited Loading

boomanaiden154 commented Mar 13, 2025

Meinersbur commented Mar 13, 2025

Meinersbur commented Feb 25, 2025 •

edited

Loading

Meinersbur commented Mar 6, 2025 •

edited

Loading

boomanaiden154 left a comment •

edited

Loading

Meinersbur Mar 11, 2025 •

edited

Loading

Meinersbur commented Mar 12, 2025 •

edited

Loading