llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.65k stars 291 forks source link

[CI] General Continuous Integration Improvements #644

Closed youngar closed 3 years ago

youngar commented 3 years ago

Hey everyone,

I would like to expand what is tested with our CI to include GCC builds and BUILD_SHARED_LIBS=on.

BUILD_SHARED_LIBS builds all libraries as shared libraries. Historically, this has mostly been used as a developer productivity flag, as it greatly speeds up linking. The real value in testing with this flag enabled is that it reveals mistakes in CMake library dependencies, such as missing dependencies and circular dependencies. There has been recent effort to increase the level of support for shared libraries to enable MLIR C/Python bindings, and supporting this flag down the line is probably a must.

From this PR, we discovered that LLVM and CIRCT both must be built with a matching settings of BUILD_SHARED_LIBS. It would not be hard to cache two versions of LLVM, but I think we should just stop building the static library version. Any opinion on this?

GCC has been making some problems for me lately, and I've had to go back and fix a couple of my PRs. I think it makes sense to add builds to catch problems before they are merged in. I'm assuming I won't have to use a separate cached build of LLVM for CIRCT GCC builds, but there is only one way to know for sure :).

Any opinions or concerns?

stephenneuendorffer commented 3 years ago

I think this is a good idea. We need to be a little careful about the size of the github cache. I'm not sure how much space we are using right now, but I think the cache might only be 5GB?

teqdruid commented 3 years ago

Yep, 5GB but that may be per workflow rather than per repo as the documentation says. Cached builds from one workflow don't get used in another. Also, the windows llvm cache is pretty big and I haven't noticed any cache misses which I've attributed to cache eviction.

The other concern is that we have a limited number of builder agents, which is why I've been doing additional checks nightly rather than on each PR and push to main. (I've had builds queuing for quite some time during LLVM submodule updates.) I think doing one compile in the PR gate is sufficient to cache most of the dumb stuff, and a sparse matrix test nightly is consistent with what LLVM does.

I think it's reasonable for the PR gate build check to be with shared libs to catch more dumb stuff in the cmake files, but I would prefer to stick with clang.

Historically, this has mostly been used as a developer productivity flag, as it greatly speeds up linking.

I used to do this for that reason, but I found that gdb takes forever to start with shared lib linking. Switching to 'lld' gets me both fast compiles and quick gdb start times.

youngar commented 3 years ago

That is really interesting, official doc here. From du on my mac, both Debug builds:

6.7G    ./build-shared
 16G    ./build-static

Maybe they are nice enough to let us keep 1 oversized cache entry? I was going to bring up using ccache for CIRCT builds, but if it ends up evicting LLVM then it is not an option.

we have a limited number of builder agents, which is why I've been doing additional checks nightly rather than on each PR and push to main

Thanks for pointing that out. I agree it makes sense to put GCC builds in to nightly

Edit: PR https://github.com/llvm/circt/pull/646 opened for this.

Total build time LLVM Cache Size
Before 8m 16s ~883 MB
After 6m 15s ~293 MB
youngar commented 3 years ago

I would like to modify the Github actions for the nightly integration tests to only run on the the main CIRCT repo. This should prevent them from running every night on everyone's forks. No one relies on nightly integration builds on their forks, right?

It looks like we can't disable it on forks from the schedule trigger, which is very annoying. I can disable it as a conditional on the workflow's job, but I'm guessing that will prevent the whole workflow from working with manual trigger in forks. Maybe I can try splitting this up into 2 worksflow: integration testing, and a nightly workflow which triggers the integration workflow. I can play around with this and see if I can get something working.

teqdruid commented 3 years ago

I've just disabled it in my fork manually on the actions page.

youngar commented 3 years ago

When I did that I found I couldn't manually launch the workflow anymore.

Edit: PR https://github.com/llvm/circt/pull/658 opened for this.

youngar commented 3 years ago

I opened PR https://github.com/llvm/circt/pull/659 to skip the "Build LLVM" part of a PR build, I don't think its necessary any more.

youngar commented 3 years ago

PR for adding GCC to the PR gating builds here: https://github.com/llvm/circt/pull/702. The PR switches out one of the two clang builds for gcc.

youngar commented 3 years ago

PR for expanding the nightly testing here: https://github.com/llvm/circt/pull/712.