Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 14, 2025
Merged

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

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f8b5bf
[libclc] Fix commands in compile_to_bc are executed sequentially
wenju-he Mar 11, 2025
3181480
revert ARG_DEPENDENCIES change
wenju-he Mar 11, 2025
ea2f503
add FIXME about DEPENDS_EXPLICIT_ONLY
wenju-he Mar 18, 2025
603255c
update comment
wenju-he Mar 18, 2025
aed94cf
Merge branch 'main' into libclc-compile-target
wenju-he Apr 10, 2025
952e8c1
remove -as target, add back to DEPENDS of builtins_comp_lib_tgt
wenju-he Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions libclc/cmake/modules/AddLibclc.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Compiles an OpenCL C - or assembles an LL file - to bytecode
#
# Arguments:
# * TARGET <string>
# Custom target to create
# * TRIPLE <string>
# Target triple for which to compile the bytecode file.
# * INPUT <string>
Expand All @@ -17,7 +19,7 @@
function(compile_to_bc)
cmake_parse_arguments(ARG
""
"TRIPLE;INPUT;OUTPUT"
"TARGET;TRIPLE;INPUT;OUTPUT"
"EXTRA_OPTS;DEPENDENCIES"
${ARGN}
)
Expand Down Expand Up @@ -63,13 +65,15 @@ function(compile_to_bc)
${ARG_DEPENDENCIES}
DEPFILE ${ARG_OUTPUT}.d
)
add_custom_target( ${ARG_TARGET} DEPENDS ${ARG_OUTPUT}${TMP_SUFFIX} )
Copy link
Contributor

@frasercrmck frasercrmck Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wenju-he wenju-he Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@frasercrmck frasercrmck Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wenju-he wenju-he Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if( ${FILE_EXT} STREQUAL ".ll" )
add_custom_command(
OUTPUT ${ARG_OUTPUT}
COMMAND ${llvm-as_exe} -o ${ARG_OUTPUT} ${ARG_OUTPUT}${TMP_SUFFIX}
DEPENDS ${llvm-as_target} ${ARG_OUTPUT}${TMP_SUFFIX}
)
add_custom_target( ${ARG_TARGET}-as DEPENDS ${ARG_OUTPUT} )
endif()
endfunction()

Expand Down Expand Up @@ -227,6 +231,7 @@ function(add_libclc_builtin_set)

set( bytecode_files )
set( bytecode_ir_files )
set( compile_tgts )
foreach( file IN LISTS ARG_GEN_FILES ARG_LIB_FILES )
# We need to take each file and produce an absolute input file, as well
# as a unique architecture-specific output file. We deal with a mix of
Expand Down Expand Up @@ -256,19 +261,25 @@ function(add_libclc_builtin_set)

get_filename_component( file_dir ${file} DIRECTORY )

string( REPLACE "/" "-" replaced ${file} )
set( tgt compile_tgt-${ARG_ARCH_SUFFIX}${replaced})

compile_to_bc(
TARGET ${tgt}
TRIPLE ${ARG_TRIPLE}
INPUT ${input_file}
OUTPUT ${output_file}
EXTRA_OPTS -fno-builtin -nostdlib
"${ARG_COMPILE_FLAGS}" -I${CMAKE_CURRENT_SOURCE_DIR}/${file_dir}
DEPENDENCIES ${input_file_dep}
)
list( APPEND compile_tgts ${tgt} )

# Collect all files originating in LLVM IR separately
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 )
Copy link
Contributor

@frasercrmck frasercrmck Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wenju-he wenju-he Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, removed

else()
list( APPEND bytecode_files ${output_file} )
endif()
Expand All @@ -283,7 +294,7 @@ function(add_libclc_builtin_set)

set( builtins_comp_lib_tgt builtins.comp.${ARG_ARCH_SUFFIX} )
add_custom_target( ${builtins_comp_lib_tgt}
DEPENDS ${bytecode_files}
DEPENDS ${compile_tgts}
Copy link
Contributor

@frasercrmck frasercrmck Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wenju-he wenju-he Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wenju-he wenju-he Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, added back ${bytecode_files}

)
set_target_properties( ${builtins_comp_lib_tgt} PROPERTIES FOLDER "libclc/Device IR/Comp" )

Expand Down