Skip to content

[libclc] Skip opt command if opt_flags is empty #130882

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

Closed
wenju-he wants to merge 6 commits into from
Closed

[libclc] Skip opt command if opt_flags is empty #130882

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1727cb4
[libclc] Skip opt command if opt_flags is empty
wenju-he Mar 12, 2025
d3543f3
unconditionally add builtins_opt_lib_tgt, which is empty if OPT_FLAGS…
wenju-he Mar 18, 2025
e02feed
update comment
wenju-he Mar 18, 2025
cb61d08
refine
wenju-he Mar 18, 2025
e7cecc2
unify PROPERTIES FOLDER
wenju-he Mar 18, 2025
36ea924
NOT ARG_OPT_FLAGS
wenju-he Mar 20, 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
39 changes: 25 additions & 14 deletions libclc/cmake/modules/AddLibclc.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,21 +342,32 @@ function(add_libclc_builtin_set)

set( builtins_opt_lib_tgt builtins.opt.${ARG_ARCH_SUFFIX} )

# Add opt target
add_custom_command( OUTPUT ${builtins_opt_lib_tgt}.bc
COMMAND ${opt_exe} ${ARG_OPT_FLAGS} -o ${builtins_opt_lib_tgt}.bc
${builtins_link_lib}
DEPENDS ${opt_target} ${builtins_link_lib} ${builtins_link_lib_tgt}
)
add_custom_target( ${builtins_opt_lib_tgt}
ALL DEPENDS ${builtins_opt_lib_tgt}.bc
)
set_target_properties( ${builtins_opt_lib_tgt} PROPERTIES
TARGET_FILE ${CMAKE_CURRENT_BINARY_DIR}/${builtins_opt_lib_tgt}.bc
FOLDER "libclc/Device IR/Opt"
)
if( ${ARG_OPT_FLAGS} STREQUAL "" )
Copy link
Contributor

@frasercrmck frasercrmck Mar 20, 2025

Choose a reason for hiding this comment

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

I just realised this code isn't triggered upstream. I take it you have a downstream making use of it?

I tried it locally and this needs to be if( "${ARG_OPT_FLAGS} STREQUAL "" ) or maybe better yet if ( NOT ARG_OPT_FLAGS ). The code as-is generates an error if I pass in an empty OPT_FLAGS:

CMake Error at /llvm-project/libclc/cmake/modules/AddLibclc.cmake:345 (if):
  if given arguments:

    "STREQUAL" ""

  Unknown arguments specified

Copy link
Contributor Author

@wenju-he wenju-he Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just realised this code isn't triggered upstream. I take it you have a downstream making use of it?

there is a case in upstream LLVM that opt flag is empty:

llvm-project/libclc/CMakeLists.txt

Line 353 in ad5cac3

set( opt_flags )

I tried it locally and this needs to be if( "${ARG_OPT_FLAGS} STREQUAL "" ) or maybe better yet if ( NOT ARG_OPT_FLAGS ). The code as-is generates an error if I pass in an empty OPT_FLAGS:

CMake Error at /llvm-project/libclc/cmake/modules/AddLibclc.cmake:345 (if):
  if given arguments:

    "STREQUAL" ""

  Unknown arguments specified

Thanks for the testing. Changed to if ( NOT ARG_OPT_FLAGS )
I don't know what happened, a few days ago I did observe that bitcode size become smaller or larger when opt flag is empty.

Copy link
Contributor Author

@wenju-he wenju-he Mar 20, 2025

Choose a reason for hiding this comment

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

I remembered. I was testing for amdgpu arch. I just switched the code between if and else blocks for testing, and didn't set opt_flags to empty. So the check was wrong at the beginning.

Copy link
Contributor

@frasercrmck frasercrmck Mar 21, 2025

Choose a reason for hiding this comment

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

That's true about SPIR-V, but those targets exit early before we reach the opt target.

If there's no plan for this code to be used in production, I'm not sure it's worth adding this extra logic which needs to be maintained. If it's just for being played around with in testing then the extra time it takes to build isn't so bad? Unless I'm misunderstanding the problem?

Copy link
Contributor Author

@wenju-he wenju-he Mar 21, 2025

Choose a reason for hiding this comment

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

just curious would this Pr unifies the this code and avoid the early exit and duplicate code for prepare target?

Copy link
Contributor Author

@wenju-he wenju-he Mar 22, 2025

Choose a reason for hiding this comment

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

let me know if it worth to update PR to unifies the code path and avoids early exit. Otherwise I'll close this PR.

The original request of this PR is to disable opt -O3. But things has changed and the request is irrelevant.
Though in practice it might still make sense to apply the PR to avoid code diverge like for SPIR-V.

Copy link
Contributor

@frasercrmck frasercrmck Mar 24, 2025

Choose a reason for hiding this comment

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

Yes, that might be good actually. Then if SPIR-V did ever want to apply optimizations, it could do so, and it ensures this path is tested.

Note though we'd still need the special-case after the opt target, because SPIR-V will continue to want to run llvm-spirv and exit. It also doesn't run the ${prepare_builtins_exe} utility so needs to exit before that point. I don't know enough about prepare-builtins to say whether SPIR-V could also make use of it: it seems to strip opencl.ocl.version metadata and set every external definition to linkonce_odr linkage. I'd need to investigate what's going on there.

# Add empty opt target.
add_custom_target( ${builtins_opt_lib_tgt} ALL )
set_target_properties( ${builtins_opt_lib_tgt} PROPERTIES
FOLDER "libclc/Device IR/Opt"
)
add_dependencies( ${builtins_opt_lib_tgt} ${builtins_link_lib_tgt} )

set( builtins_opt_lib ${builtins_link_lib} )
else()
# Add opt target
add_custom_command( OUTPUT ${builtins_opt_lib_tgt}.bc
COMMAND ${opt_exe} ${ARG_OPT_FLAGS} -o ${builtins_opt_lib_tgt}.bc
${builtins_link_lib}
DEPENDS ${opt_target} ${builtins_link_lib} ${builtins_link_lib_tgt}
)
add_custom_target( ${builtins_opt_lib_tgt}
ALL DEPENDS ${builtins_opt_lib_tgt}.bc
)
set_target_properties( ${builtins_opt_lib_tgt} PROPERTIES
TARGET_FILE ${CMAKE_CURRENT_BINARY_DIR}/${builtins_opt_lib_tgt}.bc
FOLDER "libclc/Device IR/Opt"
)

set( builtins_opt_lib $<TARGET_PROPERTY:${builtins_opt_lib_tgt},TARGET_FILE> )
set( builtins_opt_lib $<TARGET_PROPERTY:${builtins_opt_lib_tgt},TARGET_FILE> )
endif()

# Add prepare target
set( obj_suffix ${ARG_ARCH_SUFFIX}.bc )
Expand Down