[libclc] Fix commands in compile_to_bc are executed sequentially #130755

wenju-he · 2025-03-11T11:25:53Z

In libclc, we observe that compiling OpenCL source files to bitcode is
executed sequentially on Windows, which increases debug build time by
about an hour.
add_custom_command may introduce additional implicit dependencies, see
https://gitlab.kitware.com/cmake/cmake/-/issues/17097
This PR adds a target for each command, enabling parallel builds of
OpenCL source files.
CMake 3.27 has fixed above issue with DEPENDS_EXPLICIT_ONLY. When LLVM
upgrades cmake vertion to 3.7, we can switch to DEPENDS_EXPLICIT_ONLY.

In libclc, we observe that compiling OpenCL source files to bitcode is executed sequentially on Windows, which increases debug build time by about an hour. add_custom_command may introduce additional implicit dependencies, see https://gitlab.kitware.com/cmake/cmake/-/issues/17097 This PR adds a target for each command and enables parallel builds of OpenCL source files.

wenju-he · 2025-03-11T23:54:13Z

@frasercrmck could you please review? thanks

frasercrmck · 2025-03-17T11:10:39Z

Am I right in thinking that CMake 3.27's DEPENDS_EXPLICIT_ONLY would also fix this? If so it might be worth documenting this explicitly, both in the PR/commit and in the code? We might be able to refactor this in the future, when LLVM updates its minimum version to 3.27.

wenju-he · 2025-03-18T10:18:30Z

Am I right in thinking that CMake 3.27's DEPENDS_EXPLICIT_ONLY would also fix this? If so it might be worth documenting this explicitly, both in the PR/commit and in the code? We might be able to refactor this in the future, when LLVM updates its minimum version to 3.27.

done.
I noticed DEPENDS_EXPLICIT_ONLY in the cmake issue link above, but I didn't test it.
I have just tested DEPENDS_EXPLICIT_ONLY on windows, it can indeed fix the sequential build issue.

frasercrmck

Sorry, I didn't realise I hadn't published this review from last week!

frasercrmck · 2025-03-17T11:04:40Z

libclc/cmake/modules/AddLibclc.cmake

      ${ARG_DEPENDENCIES}
    DEPFILE ${ARG_OUTPUT}.d
  )
+  add_custom_target( ${ARG_TARGET} DEPENDS ${ARG_OUTPUT}${TMP_SUFFIX} )


I don't think the (possibly) "temp" output (in the case of .ll files) should be given the honour of the ARG_TARGET target. The final output of this function should unconditionally have ARG_TARGET. That saves doing -as workarounds in other parts of the code. The TARGET name the user passes should be definitive, in other words.

Do we strictly need a target for the intermediate step too? If we do, we could have ${ARG_TARGET}${TMP_SUFFIX} here. Then non-IR files get ARG_TARGET target, and then in the IR block below we create ARG_TARGET for that case.

I have simplified the code by removing ${ARG_TARGET}-as
The issue we observing is that .cl files are compiled sequentially. We don't see problem with .ll file, so we can leave it as-is.

Ah yes that's much simpler.

I'm half wondering if this isn't only for enabling parallel execution but is technically required for correct behaviour. I know CMake can behave strangely when only depending upon files and not targets, which is that we're doing before this PR. Though even with this PR, we're still implicitly depending on files coming out of the llvm-as step. That's what makes me a bit nervous. This is still better than what we've got so maybe we can keep an eye on it and see if we have any parallel build issues stemming from .ll files, and we can adjust as needed.

I think I'll also post a follow-up which handles empty arguments. I was going to ask about handling an empty TARGET but noticed there's nothing for empty INPUT or OUTPUT so maybe that's best left handled later in one go.

This is still better than what we've got so maybe we can keep an eye on it and see if we have any parallel build issues stemming from .ll files, and we can adjust as needed.

yeah, I agree

frasercrmck · 2025-03-17T11:05:33Z

libclc/cmake/modules/AddLibclc.cmake

    get_filename_component( file_ext ${file} EXT )
    if( ${file_ext} STREQUAL ".ll" )
      list( APPEND bytecode_ir_files ${output_file} )
+      list( APPEND compile_tgts ${tgt}-as )


See above - we wouldn't need this case if we could rely on a consistent target name.

done, removed

frasercrmck · 2025-03-17T11:06:38Z

libclc/cmake/modules/AddLibclc.cmake

  set( builtins_comp_lib_tgt builtins.comp.${ARG_ARCH_SUFFIX} )
  add_custom_target( ${builtins_comp_lib_tgt}
-    DEPENDS ${bytecode_files}
+    DEPENDS ${compile_tgts}


Do we not need to depend on the targets and the files? CMake dependencies are strange.

thanks, I'll add ${bytecode_files} depends back

done, added back ${bytecode_files}

wenju-he · 2025-03-26T22:59:32Z

@frasercrmck should we check CMAKE_VERSION and use DEPENDS_EXPLICIT_ONLY if the version is 3.27 or higher? I suppose DEPENDS_EXPLICIT_ONLY would be more light-weighted. And we need to keep both DEPENDS_EXPLICIT_ONLY and adding new target path, until llvm uplifts cmake version to 3.27.

frasercrmck

On my build this increases the number of targets ninja -t targets from 19K to 28K. That's building all targets. I'm not sure what else we could do about that and whether it's a problem.

frasercrmck · 2025-04-10T09:14:50Z

libclc/cmake/modules/AddLibclc.cmake

      ${ARG_DEPENDENCIES}
    DEPFILE ${ARG_OUTPUT}.d
  )
+  add_custom_target( ${ARG_TARGET} DEPENDS ${ARG_OUTPUT}${TMP_SUFFIX} )


Ah yes that's much simpler.

I'm half wondering if this isn't only for enabling parallel execution but is technically required for correct behaviour. I know CMake can behave strangely when only depending upon files and not targets, which is that we're doing before this PR. Though even with this PR, we're still implicitly depending on files coming out of the llvm-as step. That's what makes me a bit nervous. This is still better than what we've got so maybe we can keep an eye on it and see if we have any parallel build issues stemming from .ll files, and we can adjust as needed.

I think I'll also post a follow-up which handles empty arguments. I was going to ask about handling an empty TARGET but noticed there's nothing for empty INPUT or OUTPUT so maybe that's best left handled later in one go.

wenju-he · 2025-04-10T23:49:28Z

On my build this increases the number of targets ninja -t targets from 19K to 28K. That's building all targets. I'm not sure what else we could do about that and whether it's a problem.

yes, the number of targets increase significantly with this approach. But as far as I know, we don't have compilation correctness issue regarding the increase of the number of targets.
The only alternative approach is DEPENDS_EXPLICIT_ONLY. I can add DEPENDS_EXPLICIT_ONLY support to this PR if cmake version is 3.27 or higher.

wenju-he added 2 commits March 11, 2025 04:24

revert ARG_DEPENDENCIES change

3181480

jsji requested a review from frasercrmck March 17, 2025 00:17

add FIXME about DEPENDS_EXPLICIT_ONLY

ea2f503

update comment

603255c

frasercrmck reviewed Mar 26, 2025

View reviewed changes

wenju-he added 2 commits April 10, 2025 01:02

Merge branch 'main' into libclc-compile-target

aed94cf

remove -as target, add back to DEPENDS of builtins_comp_lib_tgt

952e8c1

wenju-he requested a review from frasercrmck April 10, 2025 08:10

frasercrmck reviewed Apr 10, 2025

View reviewed changes

wenju-he requested a review from frasercrmck April 11, 2025 00:08

frasercrmck approved these changes Apr 14, 2025

View reviewed changes

frasercrmck merged commit 0c21d6b into llvm:main Apr 14, 2025
9 checks passed

wenju-he deleted the libclc-compile-target branch April 14, 2025 13:19

[libclc] Fix commands in compile_to_bc are executed sequentially #130755

[libclc] Fix commands in compile_to_bc are executed sequentially #130755

Uh oh!

Conversation

wenju-he commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wenju-he commented Mar 11, 2025

Uh oh!

frasercrmck commented Mar 17, 2025

Uh oh!

wenju-he commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frasercrmck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenju-he commented Mar 26, 2025

Uh oh!

frasercrmck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenju-he commented Apr 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wenju-he commented Mar 11, 2025 •

edited

Loading

wenju-he commented Mar 18, 2025 •

edited

Loading