[CI][Github] Enable CIR CI build and test #147430
Conversation
This change modifies CI scripts to add a pseudo-project for CIR and detect when CIR-specific files are modified. It also enables building clang with CIR enabled whenever both the clang and mlir projects are being built and enables CIR testing when the mlir project is modified. Building and testing CIR is only enabled on Linux at this time, as CIR doesn't properly support Windows or MacOS yet.
andykaylor
requested review from
tstellar,
lnihlen,
joker-eph,
bcardosolopes,
erichkeane and
boomanaiden154
|
@llvm/pr-subscribers-github-workflow Author: Andy Kaylor (andykaylor) ChangesThis change modifies CI scripts to add a pseudo-project for CIR and detect when CIR-specific files are modified. It also enables building clang with CIR enabled whenever both the clang and mlir projects are being built and enables CIR testing when the mlir project is modified. Building and testing CIR is only enabled on Linux at this time, as CIR doesn't properly support Windows or MacOS yet. Full diff: https://github.com/llvm/llvm-project/pull/147430.diff 3 Files Affected:
diff --git a/.ci/compute_projects.py b/.ci/compute_projects.py
index c3cf714ce6c10..25e8bf8eaf981 100644
--- a/.ci/compute_projects.py
+++ b/.ci/compute_projects.py
@@ -19,6 +19,7 @@
PROJECT_DEPENDENCIES = {
"llvm": set(),
"clang": {"llvm"},
+ "CIR": {"clang", "mlir"},
"bolt": {"clang", "lld", "llvm"},
"clang-tools-extra": {"clang", "llvm"},
"compiler-rt": {"clang", "lld"},
@@ -50,11 +51,15 @@
"lld": {"bolt", "cross-project-tests"},
# TODO(issues/132795): LLDB should be enabled on clang changes.
"clang": {"clang-tools-extra", "cross-project-tests"},
- "mlir": {"flang"},
+ "mlir": {
+ "CIR",
+ "flang",
+ },
# Test everything if ci scripts are changed.
".ci": {
"llvm",
"clang",
+ "CIR",
"lld",
"lldb",
"bolt",
@@ -128,6 +133,7 @@
"lldb": "check-lldb",
"llvm": "check-llvm",
"clang": "check-clang",
+ "CIR": "check-clang-cir",
"bolt": "check-bolt",
"lld": "check-lld",
"flang": "check-flang",
@@ -192,7 +198,12 @@ def _compute_projects_to_test(modified_projects: Set[str], platform: str) -> Set
def _compute_projects_to_build(
projects_to_test: Set[str], runtimes: Set[str]
) -> Set[str]:
- return _add_dependencies(projects_to_test, runtimes)
+ projects_with_deps = _add_dependencies(projects_to_test, runtimes)
+ # CIR is used as a pseudo-project in this script. We detect modifications
+ # to clang's CIR-specific subdirectories and add CIR as a modified project
+ # if a file in these directories is modified, but we need to remove it
+ # explicitly here.
+ return projects_with_deps - {"CIR"}
def _compute_project_check_targets(projects_to_test: Set[str]) -> Set[str]:
@@ -247,6 +258,14 @@ def _get_modified_projects(modified_files: list[str]) -> Set[str]:
# capacity.
if len(path_parts) > 3 and path_parts[:3] == ("llvm", "utils", "gn"):
continue
+ # If the file is in the clang/lib/CIR directory, add the CIR project.
+ if (len(path_parts) > 3 and
+ (path_parts[:3] == ("clang", "lib", "CIR") or
+ path_parts[:3] == ("clang", "test", "CIR") or
+ path_parts[:4] == ("clang", "include", "clang", "CIR"))):
+ modified_projects.add("clang")
+ modified_projects.add("CIR")
+ continue
modified_projects.add(pathlib.Path(modified_file).parts[0])
return modified_projects
@@ -267,6 +286,10 @@ def get_env_variables(modified_files: list[str], platform: str) -> Set[str]:
runtimes_check_targets_needs_reconfig = _compute_project_check_targets(
runtimes_to_test_needs_reconfig
)
+
+ # Check if both clang and mlir are in projects_to_build to enable CIR
+ enable_cir = "ON" if "clang" in projects_to_build and "mlir" in projects_to_build else "OFF"
+
# We use a semicolon to separate the projects/runtimes as they get passed
# to the CMake invocation and thus we need to use the CMake list separator
# (;). We use spaces to separate the check targets as they end up getting
@@ -279,6 +302,7 @@ def get_env_variables(modified_files: list[str], platform: str) -> Set[str]:
"runtimes_check_targets_needs_reconfig": " ".join(
sorted(runtimes_check_targets_needs_reconfig)
),
+ "enable_cir": enable_cir,
}
diff --git a/.ci/monolithic-linux.sh b/.ci/monolithic-linux.sh
index 8d1faab13986c..0d35becdd8c8f 100755
--- a/.ci/monolithic-linux.sh
+++ b/.ci/monolithic-linux.sh
@@ -53,6 +53,7 @@ targets="${2}"
runtimes="${3}"
runtime_targets="${4}"
runtime_targets_needs_reconfig="${5}"
+enable_cir="${6}"
lit_args="-v --xunit-xml-output ${BUILD_DIR}/test-results.xml --use-unique-output-file-name --timeout=1200 --time-tests"
@@ -72,6 +73,7 @@ cmake -S "${MONOREPO_ROOT}"/llvm -B "${BUILD_DIR}" \
-G Ninja \
-D CMAKE_PREFIX_PATH="${HOME}/.local" \
-D CMAKE_BUILD_TYPE=Release \
+ -D CLANG_ENABLE_CIR=${enable_cir} \
-D LLVM_ENABLE_ASSERTIONS=ON \
-D LLVM_BUILD_EXAMPLES=ON \
-D COMPILER_RT_BUILD_LIBFUZZER=OFF \
diff --git a/.github/workflows/premerge.yaml b/.github/workflows/premerge.yaml
index 4435a3e905768..6ccc9f607ccf7 100644
--- a/.github/workflows/premerge.yaml
+++ b/.github/workflows/premerge.yaml
@@ -61,7 +61,7 @@ jobs:
export CC=/opt/llvm/bin/clang
export CXX=/opt/llvm/bin/clang++
- ./.ci/monolithic-linux.sh "${projects_to_build}" "${project_check_targets}" "${runtimes_to_build}" "${runtimes_check_targets}" "${runtimes_check_targets_needs_reconfig}"
+ ./.ci/monolithic-linux.sh "${projects_to_build}" "${project_check_targets}" "${runtimes_to_build}" "${runtimes_check_targets}" "${runtimes_check_targets_needs_reconfig}" "${enable_cir}"
- name: Upload Artifacts
uses: actions/upload-artifact@65c4c4a1ddee5b72f698fdd19549f0f0fb45cf08 # v4.6.0
with:
|
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
This PR needs to update the tests in compute_projects_tests.py and potentially add new ones if relevant.
Do you know how much additional time it takes to test ClangIR, particularly on changes to MLIR? MLIR changes are frequent and adding a clang build/checks has the potential to significantly increase the overall premerge time.
Thanks! I hadn't noticed Triggering at least a build with CIR enabled when MLIR is changed is one of the main motivators of this PR. We've had a couple of instances recently where MLIR changes broke CIR. Currently the Very long term, I would expect this to be around the size of the full set of clang codegen tests, 5326 tests which take nearly 10 minutes to run. If that becomes a problem, we can look at defining a smaller set of CIR tests to run with MLIR changes. Basically, I just want enough to be run to verify that CIR generation and lowering from CIR to LLVM IR are working in general. We wouldn't need to run the full suite for MLIR changes. I don't have data for the extra build time of building clang with CIR enabled. Relative to building just clang and its dependent projects, there is about a 30% increase, but most of that is because we have to build MLIR, so MLIR itself wouldn't incur that magnitude of overhead. |
Currently, yes. I need to set up a custom workflow for running it and some other misc CI things but haven't gotten around to it. It doesn't fit well into any of the current testing paradigms.
I didn't realize they were that fast. That makes me a bit more comfortable with this. What sort of machine are you getting the 30 seconds figure from?
They run quite a bit faster than that on the CI machines from what I remember. All of
It's not just building clang with CIR enabled. Currently for MLIR changes we aren't building clang. After this patch we will be (and with CIR enabled). Theoretically we get decent build caching and what not (once we turn it on), but it's more than just the overhead of adding CIR to an existing clang build/test run. If you could get numbers for how long the premerge pipeline for MLIR takes before and after this change, that would make me a lot more comfortable with this patch. |
Can you split this in another PR? That deserves its own consideration and tradeoff. For example clang decided that they wouldn't build flang even though flang is using the clang driver because the amount of failures compared to the cost wasn't worth it. |
Also that is definitely desirable to split the test-suite in CI between the uni-testing for the "MLIR integration" and the rest. That also shouldn't be hard I think? |
That was running on a single CPU AMD EPYC 7513 (Milan). I ran the tests (both CIR and clang codegen) with a debug build and all targets enabled, but otherwise I'm not sure why the clang codegen tests took so much longer for me.
Are you sure? Because even before this change flang has a dependency on clang in |
Sure. I can make that a separate PR. I agree that changes to MLIR core are all we need to worry about now, though when we start upstreaming the code to lower CIR to other dialects (besides the llvm dialect) that may expand, depending on what you consider "core". In any case, I don't want to sign myself up for implementing that split. Creating a separate target for reduced testing would be simple, but looking again at the scripts, I don't think we could provide a different testing target for "CIR as a dependency of MLIR" than we use for CIR in general without revising the way the script generates the testing targets. That could be done, of course, but we're a long way from having enough CIR tests for it to be necessary. Is that something that would be desirable for other components as well? |
Sure, I'll do it.
I didn't quite get what you mean by "that" here? |
Sorry. I meant having a different set of tests run for testing as a test dependency than are run when the project itself is modified. For instance, when CIR files are modified we want to test the "check-clang-cir" target, but when CIR testing is being run because the MLIR project was modified maybe we want to test something like "check-clang-cir-min". My question was whether there would be other cases where we'd want to run a subset of tests in the dependency case. |
|
@andykaylor / @joker-eph Are you able to come to the infra area team meeting this week? I'd like to ensure we have community consensus on the additional compute resource, latency costs, and additional maintenance/error burden and I think that's the most reasonable forum to do it. Although there really shouldn't be any concerns if we're just enabling CIR checks on CIR changes, probably not a bad idea to start the discussion now. |
I can attend. |
This change modifies CI scripts to add a pseudo-project for CIR and detect when CIR-specific files are modified. It also enables building clang with CIR enabled whenever both the clang and mlir projects are being built.
Building and testing CIR is only enabled on Linux at this time, as CIR doesn't properly support Windows or MacOS yet.