Thanks! LGTM.

In D125847#3528111, @aeubanks wrote:

I'm still looking into a whole program devirtualization bug that only repros with opaque pointers, and there are still potential performance issues to look into

Is there any issues currently if we just always use opaque pointer in LTO?

LGTM. Can you add a test for your usecase?

This breaks darwin bot: https://green.lab.llvm.org/green/job/clang-stage1-RA/29421/

Maybe it is easier if you only warn if multiple different platforms are specified? You can preserve the last one wins for the same platform.
Or even more specifically, the only supported configuration is macOS + macCatalyst and you can only toss out a warning if this configuration is used.

Add win32.

Not sure about win32 situation as it is also guarded in compiler-rt/lib/asan/CMakeLists.txt:

if (NOT WIN32 AND NOT APPLE)
   list(APPEND ASAN_SOURCES
     asan_rtl_x86_64.S
     asan_interceptors_vfork.S
     )
 endif()

This is actually fixed in f51d7e4bae9e861e711ad9711599456fc2f1bbca before this is reviewed. I will just close this.

This breaks implicit module on macOS bots: https://green.lab.llvm.org/green/job/lldb-cmake/42098/console

In D118352#3368922, @ChuanqiXu wrote:

In D118352#3368919, @phosek wrote:

We're also seeing this issue on our Mac bots, is it possible to revert it?

I've reverted it. Since this is the new feature, I think it wouldn't so hurry to land it. BTW, I think it would be helpful to provide more information to fix it.

Just a note, this also breaks implicit module build as the newly added header indirectly includes llvm/IR/Attributes.inc which needs to be generated first. If you do a module build (-DLLVM_ENABLE_MODULES=ON), you are likely to get error:

09:05:38 While building module 'LLVM_MC' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/lib/MC/MCAsmBackend.cpp:9:
09:05:38 While building module 'LLVM_IR' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:49:
09:05:38 While building module 'LLVM_intrinsic_gen' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/VectorBuilder.h:18:
09:05:38 In file included from <module-includes>:1:
09:05:38 In file included from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/Argument.h:17:
09:05:38 /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/Attributes.h:75:14: fatal error: 'llvm/IR/Attributes.inc' file not found
09:05:38     #include "llvm/IR/Attributes.inc"
09:05:38              ^~~~~~~~~~~~~~~~~~~~~~~~
09:05:38 While building module 'LLVM_MC' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/lib/MC/MCAsmBackend.cpp:9:
09:05:38 While building module 'LLVM_IR' imported from /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/MC/MCPseudoProbe.h:49:
09:05:38 In file included from <module-includes>:62:
09:05:38 /System/Volumes/Data/jenkins/workspace/apple-clang-stage2-configure-RA_master/llvm-project/llvm/include/llvm/IR/VectorBuilder.h:18:10: fatal error: could not build module 'LLVM_intrinsic_gen'
09:05:38 #include <llvm/IR/IRBuilder.h>
09:05:38  ~~~~~~~~^

LGTM

I just saw this. I know it is a good idea to have choice during link time for the pipeline configuration and from your benchmark it also has size impact on the output. I also feel like this is going in the wrong direction as if I have part of the object files built with -O3 and part built with -Oz, I need to make a choice of unoptimized part of the program.

clang-format

Rebase to TOT

Rebase the change to TOT

Fix test name.

Address review feedback.

Rename the variables to be more accurate.

Thanks! LGTM now.

@lhames ping

Fix the rebase issue that mixed up with D114749

Address review feedback

Address review feedback

LGTM.

Few things to improve before commit:

• Drop the words like "in my case", "I", etc. from the commit message. Eg, In my case, after optimize is called, I need to access the underlying module to get information. can be rewritten as API can be used to access the underlying module to get information after optimized is called. It would sound much better.
• Please add a use-case of the API in test suite otherwise it can be removed as dead code. You can look into getting it used in llvm-lto tool and add a testcase to exercise the code path.

The change is fine. Can you elaborate a bit more about your use case? Or at least add a test for it?

Update the patch to trust user's action and update symbols to fit in the new block

Address review feedback after talking with Lang and Duncan offline.

LGTM. Can you use a better commit message?

The best solution might still be the linker to map the intermediate file to the original object file since the added file name here is still another level of indirection you need to figure out. @davide for his opinion.

In D111863#3071328, @housel wrote:

In D111863#3069279, @lhames wrote:

I think the ORC runtime provides a much more natural way to test this. Did you manage to come up with some ORC-runtime based tests in the end?

My current plan is to automate what I've been doing manually, namely running a test of C++ exception handling using llvm-jitlink, both with the default configuration (using libgcc-provided unwinding), and with libunwind LD_PRELOADed to force it as the unwinding provider.

I don't know enough about Dwarf unwinding but the implementation looks generally good. Can you please add a testcase indicating how ORCJIT is planning to use it?

LGTM.

+ @davide who might have more context of LC_CODE_SIGNITURE section

I want to do something like this for a long time. It would be good to ReleaseNote the change so people who is distributing/building with a subset of the project directories can be updated to build with top level cmake directory (@ldionne that probably include our internal build system). Some day we might be able to cmake configure with the top level llvm-project directory.

I assume this is a debug option for LTO developer that is not going for production. That is why I said LGTM without a test case. I do think it will be good to set that in ThinLTOCodeGenerator as well, probably not needed for llvm-lto2.

LGTM.

I am not familiar with offloading side of LTO driver but the change looks functionally the same to me. Thanks for doing this! Remember to update the commit message with the correct info.

In D108881#2988026, @tejohnson wrote:

In D108881#2988016, @steven_wu wrote:

In D108881#2987990, @mnadeem wrote:

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

I expect the cleanup is very very simple and that is why I am suggested to do in the same commit. I am ok with a followup fix as well.

I think there might be some high level confusion. @steven_wu earlier in the thread you said "I will do a cleanup", but I think you were asking @mnadeem to do the cleanup here in this patch.

If you didn't quite get what I mean, I am suggesting to cleanup all the reference to the old flag. Since -flto is rewrite by the driver to -flto=full, clang should not query OPT_flto anymore. All references to the old flag needs to be audited and removed while preserving the correct behavior.

In D108881#2987990, @mnadeem wrote:

In D108881#2973735, @steven_wu wrote:

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

The current change set in this review is functional change while the cleanup I want is not functional after the rewrite the old option as Alias. Once flto is the alias, there is no need to handle that in the driver and those might actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have compelling reason not to.

Hi @steven_wu any idea about the timeline?

This issue is blocking some internal work, and assuming that it will take longer to get a full fix, I would prefer it if this change could go in on its own.
Otherwise I am good with doing everything in the same commit.

In D108881#2973719, @mnadeem wrote:

In D108881#2973516, @steven_wu wrote:

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

Will you incorporate the functional changes in this patch? Or is there still a need for this change?

I will do a cleanup of parseLTOMode function since we don't need a OptPos parameter anymore. There are few minor places references OPT_flto or OPT_foffload_lto can be cleaned up too.

I agree that it might be good to change to the behavior that last arg wins but it might be simpler to set -flto to be an alias of -flto=full:

--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2086,7 +2086,7 @@ def flto_EQ : Joined<["-"], "flto=">, Flags<[CoreOption, CC1Option]>, Group<f_Gr
 def flto_EQ_jobserver : Flag<["-"], "flto=jobserver">, Group<f_Group>;
 def flto_EQ_auto : Flag<["-"], "flto=auto">, Group<f_Group>;
 def flto : Flag<["-"], "flto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
-  HelpText<"Enable LTO in 'full' mode">;
+  Alias<flto_EQ>, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
 def fno_lto : Flag<["-"], "fno-lto">, Flags<[CoreOption, CC1Option]>, Group<f_Group>,
   HelpText<"Disable LTO mode (default)">;
 def foffload_lto_EQ : Joined<["-"], "foffload-lto=">, Flags<[CoreOption]>, Group<f_Group>,

In D107799#2939530, @beanz wrote:

In D107799#2938302, @tstellar wrote:

Is there any reason why the prefix couldn't be lib/clang/14/ instead of lib/clang/14.0.0/ ?

I think keeping the full version number is good in case we rev any of the headers or libraries shipped with clang in a patch release. It is good to be able to ensure those directories don't overlap or have stale contents if clang gets installed into the same root directory.

In D107799#2938070, @phosek wrote:

That would better match other platforms, but I'd like to hear from Apple folks like @beanz whether this is something that would make sense.

I'm probably not the best person from Apple to weigh in here since I'm not actively involved with clang toolchains anymore. @steven_wu & @arphaman are probably better people to consult since they'd need to be involved.

I always thought it would be best to get Darwin to start building thin-archives for all the compiler-rt libraries, and to move to triple-based directory structure. There's no reason the compiler can't operate on thin files, and it would allow much of the driver and build system code to become unified. Which would be a huge win.

I suspect part of how we got to the place we're at now is because the driver doesn't share a lot of code between targets, and back in the old makefile days each target basically had its own makefiles for compiler-rt. CMake always tried to share logic, but became a mess trying to do _very_ different things.

It would be a big project to clean up and unify all of that, but totally worth doing.

LGTM. Thanks

Please don't use x86_64 as target or triple since it might default to some weird platform. Can you spell out a valid triple/platform?

Can you unify the duplicated code or just put the FIXME back saying need to fix the duplicated code in the future?

That is said, I am fine with the clean up but this probably doesn't resolve FIXME.

I think the FIXME is about not duplicating code from LTOCodeGenerator, not how terrible if statement looks.

LGTM in general. Just two small comments inline. Thanks!

Comment inline.

Oh, wait, I do see this warning in the build and I am not sure if this will be a problem or not:

Dependee "/Users/steven/dev/monorepo/makefile_build/projects/compiler-rt/lib/builtins/CMakeFiles/lse_builtin_symlinks.dir/DependInfo.cmake" is newer than depender "/Users/steven/dev/monorepo/makefile_build/projects/compiler-rt/lib/builtins/CMakeFiles/lse_builtin_symlinks.dir/depend.internal".

Works now. Thanks!

I still get the same error. 😿

Now I got this error when generating makefile:

-- Configuring done
CMake Error at /Users/steven/dev/monorepo/llvm-project/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:275 (add_library):
  Cannot find source file:

The error says ios_arm64e because it duplicates for every single os arm64 combination. osx_arm64, osx_arm64e, ios_arm64, ios_arm64e, etc.

No, this doesn't work:

CMake Error at /Users/steven/dev/monorepo/llvm-project/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:420 (add_custom_target):
  add_custom_target cannot create target "lse_builtin_symlinks" because
  another target with the same name already exists.  The existing target is a
  custom target created in source directory
  "/Users/steven/dev/monorepo/llvm-project/compiler-rt/lib/builtins".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  /Users/steven/dev/monorepo/llvm-project/compiler-rt/lib/builtins/CMakeLists.txt:674 (darwin_add_builtin_libraries)

In D106305#2891318, @tambre wrote:

In D106305#2890733, @steven_wu wrote:

The configured directory is rather large so let me give some more detail from the makefile generated. The custom target is generated in two of the makefiles I mentioned above, like following:

projects/compiler-rt/lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S:
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold --progress-dir=/Users/steven/makefile_build/CMakeFiles --progress-num=$(CMAKE_PROGRESS_1) "Generating outline_atomic_helpers.dir/outline_atomic_cas1_1.S"
        cd /Users/steven/makefile_build/projects/compiler-rt/lib/builtins && /usr/local/bin/cmake -E create_symlink /Users/steven/llvm-project/compiler-rt/lib/builtins/aarch64/lse.S /Users/steven/makefile_build/projects/compiler-rt/lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S

When you build the clang builtin library, it does:

$ make -j 32 clang_rt.osx VERBOSE=1
...
/usr/bin/make  -f projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins_arm64_osx.dir/build.make projects/compiler-rt/lib/        builtins/CMakeFiles/clang_rt.builtins_arm64_osx.dir/depend
/usr/bin/make  -f projects/compiler-rt/lib/builtins/CMakeFiles/clang_rt.builtins_arm64e_osx.dir/build.make projects/compiler-rt/lib/       builtins/CMakeFiles/clang_rt.builtins_arm64e_osx.dir/depend
...
CMake Error: failed to create symbolic link '/Users/steven/makefile_build/projects/compiler-rt/lib/builtins/outline_atomic_helpers.dir/outline_atomic_ldset1_3.S': file already exists
``

Thanks, I now understand why this issue happens. With Make each target gets a Make subinvocation, while in Ninja they're generated into a single buildfile and can be correctly scheduled.
This issue only happens with targets that can build multiple sub-architectures at the same time (ARM32, MIPS, PPC64) possibly causing the symlinking to run at the sametime. On Apple this includes ARM64 and x86-64, making this a bit easier to hit.

The add_custom_target() solution noted in the mailing list seems the most appropriate to force ordering but still keep parallelism as noted by @thakis. Let's go with that.

The configured directory is rather large so let me give some more detail from the makefile generated. The custom target is generated in two of the makefiles I mentioned above, like following:

projects/compiler-rt/lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S:
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold --progress-dir=/Users/steven/makefile_build/CMakeFiles --progress-num=$(CMAKE_PROGRESS_1) "Generating outline_atomic_helpers.dir/outline_atomic_cas1_1.S"
        cd /Users/steven/makefile_build/projects/compiler-rt/lib/builtins && /usr/local/bin/cmake -E create_symlink /Users/steven/llvm-project/compiler-rt/lib/builtins/aarch64/lse.S /Users/steven/makefile_build/projects/compiler-rt/lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S