Page MenuHomePhabricator

[flang][cmake] Enable the new driver by default
ClosedPublic

Authored by awarzynski on May 4 2021, 9:13 AM.

Details

Summary

With this patch:

  • FLANG_BUILD_NEW_DRIVER is set to On by default (i.e. the new driver is enabled)
  • Clang is automatically added to the list of dependencies.

By setting FLANG_BUILD_NEW_DRIVER to Off, the new driver will be
disabled and Clang will be removed from the list of dependencies.

I tried implementing this logic in Flang's top CMake script, but that's
too late for updating LLVM_ENABLE_PROJECTS (which is used to
enable/disable Clang).

Diff Detail

Event Timeline

awarzynski created this revision.May 4 2021, 9:13 AM
awarzynski requested review of this revision.May 4 2021, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2021, 9:13 AM
Meinersbur added inline comments.May 6 2021, 5:52 AM
llvm/CMakeLists.txt
78

Could you move the option into the if ("flang" IN_LIST LLVM_ENABLE_PROJECTS) condition so at least it does not appear unless flang is enabled?

84

[nit] Space between ( and FLANG_BUILD_NEW_DRIVER. While not done consistently done in the file, the patch itself could itself be consistent with itself.

86

CACHE variables are user-controlled and should not be modified. Actually, this creates a new non-cached local variables that shadows the cached variable.

From the CMake manual:

Normally projects should avoid using normal and cache variables of thesame name, as this interaction can be hard to follow.

awarzynski marked 2 inline comments as done.May 7 2021, 5:16 AM
awarzynski added inline comments.
llvm/CMakeLists.txt
86

Thanks for pointing this out. It sounds like we shouldn't be touching LLVM_ENABLE_PROJECTS at all.

We should also remove list(APPEND LLVM_ENABLE_PROJECTS "mlir"), but I'd rather send a separate patch for this.

awarzynski updated this revision to Diff 343653.May 7 2021, 5:21 AM

Address PR comments

Thank you for taking a look @Meinersbur. It sounds that instead of updating LLVM_ENABLE_PROJECTS, it will be better to generate an error when Clang is missing. What do you think?

jdoerfert accepted this revision.May 7 2021, 8:39 AM

Not a cmake expert, if @Meinersbur is happy with this we can go ahead. There was no objection on the call we discussed this nor do I see one here.

This revision is now accepted and ready to land.May 7 2021, 8:39 AM
MehdiChinoune added a subscriber: MehdiChinoune.EditedMay 7 2021, 10:32 AM

Address PR comments

Thank you for taking a look @Meinersbur. It sounds that instead of updating LLVM_ENABLE_PROJECTS, it will be better to generate an error when Clang is missing. What do you think?

You can use set(LLVM_ENABLE_PROJECTS "${LLVM_ENABLE_PROJECTS}" "clang" CACHE STRING "list of projects to build" FORCE)
to force-update it.

Meinersbur accepted this revision.May 7 2021, 12:14 PM

You can use set(LLVM_ENABLE_PROJECTS "${LLVM_ENABLE_PROJECTS}" "clang" CACHE STRING "list of projects to build" FORCE)
to force-update it.

This duplicates the option line with the setting description. Generally, I think user-controlled configuration parameters should stay user-controlled. Controlling the defaults for LLVM_TOOL_*_BUILD might be a better option.

I agree this would be matter of another patch to fix.

@MehdiChinoune , thank you for your suggestion. I agree with with @Meinersbur and prefer to merge this as is. Just waiting for the buildbot to be back online.

This revision was automatically updated to reflect the committed changes.