This is an archive of the discontinued LLVM Phabricator instance.

[runtimes] Don't override LLVM_ENABLE_PER_TARGET_RUNTIME_DIR set from llvm

Authored by DavidSpickett on Dec 7 2022, 6:29 AM.



LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is set in llvm/CMakeLists.txt
and in llvm/runtimes/CMakeLists.txt.

This meant that anything you passed down, or any platform not using
this layout yet would have it enabled despite it being OFF earlier.

To fix this, check if we have already defined the variable
and if so, use that value.

bultin_register_target I don't fully understand the purpose of.
So for now I have left it setting the value to ON. The rest will
respect what was previously set.

Diff Detail

Event Timeline

DavidSpickett created this revision.Dec 7 2022, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 6:29 AM
DavidSpickett requested review of this revision.Dec 7 2022, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 6:29 AM

Background is that I'm using the release build script to test the new library layout for armhf, and first step to that was getting a working build without any changes.

I have left the functions that do =ON alone, though maybe they need this check too. I didn't hit them in the builds I've tried.

The other thing I don't know is if we should be putting the same opt out/ins in llvm and runtimes for this setting. llvm turns it ON for BSD or Linux, except when it's on Arm. runtimes disables it for AIX. Perhaps they could be shared?

Problem with that is that I think you can build without using only the runtimes cmake, is that the case? If so, you'd need to have the opt in/outs in both places.

DavidSpickett added a subscriber: daltenty.

+ @daltenty on the off chance it makes sense to move the AIX check around. There is currently no AIX check in llvm/CMakeLists.txt

DavidSpickett added inline comments.Dec 7 2022, 6:37 AM

I don't know why the _default variable was used here, I don't think it matters too much.

With this change, if you add -DLLVM_ENABLE_PER... to your cmake command line, it will be passed down to this function as expected.

  • Move code to a macro
  • Use it in more functions that I hit testing the release build
DavidSpickett edited the summary of this revision. (Show Details)Dec 14 2022, 2:54 AM
MaskRay accepted this revision.Dec 15 2022, 12:59 AM

I do not fully understand the logic but the diff seems reasonable.

This revision is now accepted and ready to land.Dec 15 2022, 12:59 AM

Thanks for the review! I'll also wait to see what @phosek says.

The thing I'm not sure about is what level should set the default for this option.

ping @phosek Does the approach to this look good to you?

Given that this is a global setting, handling it like you suggested in does not seem the way to go.

Rebase onto main.

DavidSpickett edited the summary of this revision. (Show Details)Feb 2 2023, 3:26 AM
This revision was landed with ongoing or failed builds.Feb 2 2023, 3:33 AM
This revision was automatically updated to reflect the committed changes.