This is an archive of the discontinued LLVM Phabricator instance.

[flang] Refine how Clang dependencies are expressed #58663
ClosedPublic

Authored by Da-Viper on Jan 4 2023, 8:43 AM.

Diff Detail

Event Timeline

Da-Viper created this revision.Jan 4 2023, 8:43 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: jdoerfert. · View Herald Transcript
Da-Viper requested review of this revision.Jan 4 2023, 8:43 AM

Hello There,

This is my first contribution and I want to make sure I am doing it the right way.

This is to make the code reusable and easy to read.

I am going to

  • convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
  • explicitly name the cmake_parse_agruments arguments
  • do the same as the above for flang/FrontendTool for other flang directories

Possibly extend this to the lldb part too.

Is this the case ?

Thanks

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

flang/cmake/modules/AddFlang.cmake
66

Is this macro capable of differentiating between DYLIB and non-DYLIB builds?

flang/lib/FrontendTool/CMakeLists.txt
21

Unrelated change

Da-Viper updated this revision to Diff 486534.Jan 5 2023, 5:12 AM
Da-Viper retitled this revision from [flang] Refine how Clang dependencies are expressed #58663 to [flang] Refine how Clang dependencies are expressed #58663.
Da-Viper edited the summary of this revision. (Show Details)

Changed the name of the flang_add_library to reflect it meaning
apply changes to other files

Da-Viper marked an inline comment as not done.Jan 5 2023, 5:21 AM

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

I think I did it the right way take a look

flang/cmake/modules/AddFlang.cmake
66

It is not, It is only for dynamic libs I would update the name to reflect that

66

Looking at previous codes, if you are linking statically it is only clang-cpp that is included else it is anything that is added to the option CLANG_DYLIBS

the rest is then handled by clang_target_link_libraries see AddClang.cmake#L209

Sorry, I was meant to reply earlier!

This is my first contribution and I want to make sure I am doing it the right way.

You are - thanks for contributing :)

This is to make the code reusable and easy to read.

Sounds good! One other goal worth considering - alignment with other sub-projects.

I am going to

  • convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
  • explicitly name the cmake_parse_agruments arguments
  • do the same as the above for flang/FrontendTool for other flang directories

+1

Possibly extend this to the lldb part too.

Why not do it for all sub-projects? That would be good for consistency. I don't really contribute to other projects - you may want to ask people on Discourse first: https://discourse.llvm.org/.

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

flang/lib/Frontend/CMakeLists.txt
55 ↗(On Diff #486534)
Da-Viper updated this revision to Diff 486807.Jan 6 2023, 4:05 AM
Da-Viper marked an inline comment as not done.

Updating D140998: [flang] Refine how Clang dependencies are expressed #58663

rename CLANG_DYLIBS to CLANG_LIBS

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

flang/lib/Frontend/CMakeLists.txt
55 ↗(On Diff #486534)

I confused one for the other, reverted it

awarzynski accepted this revision.Jan 7 2023, 8:08 AM

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

That's probably the main flag to test, so we can be fairly confident that this is correct. And yes, Flang build times are very long :(

LGTM, but please address my comment in flang/lib/FrontendTool/CMakeLists.txt first. Do you have commit access for llvm-project?

This revision is now accepted and ready to land.Jan 7 2023, 8:08 AM
Da-Viper marked an inline comment as done.Apr 16 2023, 5:22 AM
Da-Viper added inline comments.
flang/lib/FrontendTool/CMakeLists.txt
21

This is now handled by clang_target_link_libraries function that is called in the add_flang_library function as it does the same thing

This revision was automatically updated to reflect the committed changes.