This is an archive of the discontinued LLVM Phabricator instance.

[CMake][NewPM] Move ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER into llvm/
ClosedPublic

Authored by aeubanks on Nov 24 2020, 8:43 PM.

Details

Summary

This allows us to use its value everywhere, rather than just clang. Some
other places, like opt and lld, will use its value soon.

The #define for it is now in llvm-config.h.

Diff Detail

Event Timeline

aeubanks created this revision.Nov 24 2020, 8:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 24 2020, 8:43 PM
aeubanks requested review of this revision.Nov 24 2020, 8:43 PM
hans accepted this revision.Dec 1 2020, 1:17 AM

Seems like a good idea to me.

From the change description:

This allows us to use its value everywhere, rather than just llvm.

Do you mean rather than just clang?

This revision is now accepted and ready to land.Dec 1 2020, 1:17 AM
rnk added inline comments.Dec 1 2020, 8:14 AM
llvm/include/llvm/Config/llvm-config.h.cmake
92

This is unrelated, but appears to be in the wrong header, it should be in config.h. It isn't namespaced, and could conflict with another project's config header.

95

This should be namespaced in LLVM_*.

Can we drop "experimental" now to shorten it? LLVM_ENABLE_NEW_PASS_MANAGER?

aeubanks edited the summary of this revision. (Show Details)Dec 1 2020, 10:13 AM

Seems like a good idea to me.

From the change description:

This allows us to use its value everywhere, rather than just llvm.

Do you mean rather than just clang?

Whoops, yup.

llvm/include/llvm/Config/llvm-config.h.cmake
95

That would mean changing the CMake variable name which would break everybody using the flag. I can add a #define alias if you want.

rnk added inline comments.Dec 1 2020, 10:28 AM
llvm/include/llvm/Config/llvm-config.h.cmake
95

You can leave the cmake option as it is, and make a new variable with the right name before the call to configure_file:

set(LLVM_ENABLE_NEW_PASS_MANAGER ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER)
aeubanks updated this revision to Diff 308713.Dec 1 2020, 10:51 AM

rename to LLVM_ENABLE_NEW_PASS_MANAGER

rnk accepted this revision.Dec 1 2020, 11:38 AM

lgtm

This revision was landed with ongoing or failed builds.Dec 1 2020, 11:43 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Dec 1 2020, 1:03 PM

It looks like this change has also enabled the new pass manager by default.

It looks like this change has also enabled the new pass manager by default.

Whoops, I'll revert and see what went wrong.

aeubanks reopened this revision.Dec 1 2020, 1:52 PM
This revision is now accepted and ready to land.Dec 1 2020, 1:52 PM
aeubanks updated this revision to Diff 308761.Dec 1 2020, 1:53 PM

set LLVM_ENABLE_NEW_PASS_MANAGER to the value of ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER, not the string itself

This revision was landed with ongoing or failed builds.Dec 1 2020, 2:02 PM
This revision was automatically updated to reflect the committed changes.