Page MenuHomePhabricator

steven_wu (Steven Wu)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 4 2014, 3:46 PM (236 w, 6 d)

Recent Activity

Tue, May 14

steven_wu added inline comments to D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends.
Tue, May 14, 1:49 PM · Restricted Project, Restricted Project
steven_wu added a comment to D60162: [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends.

Thanks for doing this. I think module flag is a good idea. Some comments inline.

Tue, May 14, 9:35 AM · Restricted Project, Restricted Project

Fri, May 10

steven_wu added a comment to D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone.

Why are you interested in expending this list?

I have a (kernel) that is compiled with -mno-red-zone and -mcmodel=large which I want to compile with -fembed-bitcode for a debugging tool that needs the bitcode.

But I can carry these patches locally for now.

Fri, May 10, 8:41 AM · Restricted Project

Tue, May 7

steven_wu added a comment to D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone.

Like I said, I am not worried that -mno-red-zone itself changes the ABI. As long as LLVM still respect the attribute the same way, it is fine. I want to consult Tim to make sure we can support re-targeting no red zone from armv7k to arm64_32.

Tue, May 7, 11:29 AM · Restricted Project
steven_wu added a comment to D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone.

@steven_wu - yeah, -mred-zone, -mno-red-zone does impact the ABI, at least on x86_64. It changes the way that the arguments are spilled and the stack layout.

Tue, May 7, 9:05 AM · Restricted Project
steven_wu added a comment to D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone.

The main concern for adding this blacklist was because of long term maintainability since the option is going to be embedded into bitcode. Looking at this specific option, I have no reason against because it doesn't affect embedded compiler flags.

Tue, May 7, 8:54 AM · Restricted Project
steven_wu added a reviewer for D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone: t.p.northover.
Tue, May 7, 8:54 AM · Restricted Project

Thu, May 2

steven_wu added a comment to D61346: [CMake] Do not use libtool on Apple platforms when building LLVM with (full) LTO..

When you are manually configuring the bootstrap, I still don't think trying to bypass libtool is the correct fix because you still need to specify the correct llvm-ar to use. The default CMAKE_AR inferred by cmake will not work. Maybe it is an option to add the control to force llvm-ar if wished, then you have two options when you manually configuring bootstrap:

Thu, May 2, 8:20 AM · Restricted Project

Wed, May 1

steven_wu updated the diff for D59945: [ObjCMetadata] Add support for reading Objective-C metadata.

Rebase the patch and did some fixup:

Wed, May 1, 9:47 AM · Restricted Project

Tue, Apr 30

steven_wu added a comment to D61346: [CMake] Do not use libtool on Apple platforms when building LLVM with (full) LTO..

If you use cmake variable CLANG_ENABLE_BOOTSTRAP, it should take care of DYLD_LIBRARY_PATH so libtool is using the just built libLTO to create static library. The setup of the DYLD_LIBRARY_PATH is just below.

Tue, Apr 30, 5:13 PM · Restricted Project
steven_wu added a comment to D61255: [ThinLTO] Make weak data symbols prevailing when they're visible to regular object.

I agree this doesn't look like the correct fix, especially the check !Sym.isExecutable() which doesn't look correct to me

The problem is that linkonce_odr functions and data should be IMO treated differently. The linkonce_odr linkage implies linker leaves only one copy of the symbol in the final image.
However it's not really a problem having multiple internal copies of linkonce_odr function because all copies follow one definition rule. This fact allows ThinLTO to efficiently optimize such function calls.
Situation however is different with linkonce_odr objects, because it's wrong to have multiple copies of linkonce_odr object because reads and writes supposed to access same memory will actually become
inconsistent. However there is still a nuance: if object is a compile time constant there can't be any write access to it, so we can safely internalize it.

Tue, Apr 30, 1:36 PM · Restricted Project
steven_wu added a comment to D61255: [ThinLTO] Make weak data symbols prevailing when they're visible to regular object.

I don't see this happening in legacy thinLTO code generator but I need to do more digging to see what is the difference. For legacy API, the static variable is not internalized or turned into available_externally.

Tue, Apr 30, 11:22 AM · Restricted Project

Mon, Apr 29

steven_wu committed rG6c9f6fd11b65: [ThinLTO] Adding architecture name into saved object filename (authored by steven_wu).
[ThinLTO] Adding architecture name into saved object filename
Mon, Apr 29, 2:39 PM
steven_wu committed rL359508: [ThinLTO] Adding architecture name into saved object filename.
[ThinLTO] Adding architecture name into saved object filename
Mon, Apr 29, 2:38 PM
steven_wu closed D60924: [ThinLTO] Adding architecture name into saved object filename.
Mon, Apr 29, 2:38 PM · Restricted Project
steven_wu added a comment to D60924: [ThinLTO] Adding architecture name into saved object filename.

ping

Mon, Apr 29, 10:57 AM · Restricted Project

Apr 19 2019

steven_wu updated the diff for D60924: [ThinLTO] Adding architecture name into saved object filename.

Add tests for the change. There is acutally an easy to test with llvm-lto

Apr 19 2019, 7:12 PM · Restricted Project
steven_wu added a comment to D60924: [ThinLTO] Adding architecture name into saved object filename.

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

Apr 19 2019, 3:17 PM · Restricted Project
steven_wu added a comment to D60924: [ThinLTO] Adding architecture name into saved object filename.

Is there a good way to test this?

Apr 19 2019, 2:55 PM · Restricted Project
steven_wu created D60924: [ThinLTO] Adding architecture name into saved object filename.
Apr 19 2019, 2:31 PM · Restricted Project

Apr 17 2019

steven_wu added a comment to D60516: [LTO] Add plumbing to save stats during LTO on Darwin..

forgot to save the inline comments.

Apr 17 2019, 10:51 AM · Restricted Project, Restricted Project
steven_wu accepted D60516: [LTO] Add plumbing to save stats during LTO on Darwin..

LGTM with one additional small comments

Apr 17 2019, 10:51 AM · Restricted Project, Restricted Project
steven_wu committed rG05a358cdcd55: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols (authored by steven_wu).
[ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 17 2019, 10:39 AM
steven_wu committed rL358601: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
[ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 17 2019, 10:39 AM
steven_wu closed D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Apr 17 2019, 10:39 AM · Restricted Project
steven_wu added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Thanks Teresa and Ben for the feedback! I am landing this change.

Apr 17 2019, 10:34 AM · Restricted Project

Apr 12 2019

steven_wu added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:

Hi Steven,

I missed that this work was being done - sorry.

Currently the legacy api does not require the IRSymtab to be written to the bitcode files so there is potentially a performance degradation due to this approach and the generated bitcode files will be larger, did you measure that?

The legacy API is not writing out IRSymtab but build that in memory during code generation. The only requirement change is that now data layout is required for thin LTO .

My understanding is that the on disk bitcode contains a binary representation of the IRSymtab and this is read into memory when loading the file (which is why the upgrade path is more expensive than the no-upgrade path). I am saying that currently the legacy api does not require the IRSymtab to be written to disk. Writing it may cost more time and make the input files bigger.

Apr 12 2019, 2:38 PM · Restricted Project

Apr 10 2019

steven_wu added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Hi Steven,

I missed that this work was being done - sorry.

Currently the legacy api does not require the IRSymtab to be written to the bitcode files so there is potentially a performance degradation due to this approach and the generated bitcode files will be larger, did you measure that?

Apr 10 2019, 8:59 AM · Restricted Project

Apr 8 2019

steven_wu updated the diff for D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Add one file that was missing from previous patch

Apr 8 2019, 2:42 PM · Restricted Project
steven_wu added a comment to D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

What part of the patch caused the need to change the internalize action to do promote+internalize in one go?

Apr 8 2019, 2:11 PM · Restricted Project
steven_wu added a reviewer for D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols: dexonsmith.
Apr 8 2019, 1:48 PM · Restricted Project
steven_wu created D60421: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Apr 8 2019, 1:46 PM · Restricted Project
steven_wu added a comment to D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

I reverted this because the old thinLTO test cases need to be updated to have data layout as a requirement to use IRSymtab. I will start a separate review to address all those issues.

Apr 8 2019, 1:18 PM · Restricted Project
steven_wu added a comment to rL357931: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

I reverted this because the old thinLTO test cases need to be updated to have data layout as a requirement to use IRSymtab. I will start a separate review to address all those issues.

Apr 8 2019, 1:18 PM
steven_wu committed rGf41e70d6eb90: Revert [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols (authored by steven_wu).
Revert [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 8 2019, 11:53 AM
steven_wu committed rL357932: Revert [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Revert [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 8 2019, 11:53 AM
steven_wu committed rG8b70a5c11e08: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols (authored by steven_wu).
[ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 8 2019, 11:23 AM
steven_wu committed rL357931: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
[ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols
Apr 8 2019, 11:22 AM
steven_wu closed D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Apr 8 2019, 11:22 AM · Restricted Project
steven_wu updated the diff for D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Review feedback.

Apr 8 2019, 9:30 AM · Restricted Project

Apr 5 2019

steven_wu added a comment to D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata..

I talked with Akira offline and we think this is probably the best approach to fix this LTO issue. I will leave others to comment if they think otherwise.

Apr 5 2019, 1:32 PM · Restricted Project, Restricted Project
steven_wu updated the diff for D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Address review feedback

Apr 5 2019, 1:28 PM · Restricted Project
steven_wu accepted D60303: [ObjC][ARC] Emit the retainRV marker as a module flag instead of named metadata..

LGTM once the clang patch is good to land.

Apr 5 2019, 12:53 PM · Restricted Project
steven_wu added inline comments to D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Apr 5 2019, 12:53 PM · Restricted Project

Apr 4 2019

steven_wu added a comment to D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
In D60226#1455485, @pcc wrote:

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

@pcc is this surprising? Typically we should just be loading it out of the bitcode file. There are cases where it has to be rebuilt, but theoretically those should be exceptions.

Yes, we normally just load it out of the bitcode file. The irsymtab only needs to be rebuilt when the version of the producer and the consumer do not match. You might be hitting the "upgrade" path here: http://llvm-cs.pcc.me.uk/lib/Object/IRSymtab.cpp#330

This is likely. I didn't bootstrap the build. I will try again with a matching version.

Apr 4 2019, 6:09 PM · Restricted Project
steven_wu added a comment to D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
In D60226#1455485, @pcc wrote:

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

@pcc is this surprising? Typically we should just be loading it out of the bitcode file. There are cases where it has to be rebuilt, but theoretically those should be exceptions.

Yes, we normally just load it out of the bitcode file. The irsymtab only needs to be rebuilt when the version of the producer and the consumer do not match. You might be hitting the "upgrade" path here: http://llvm-cs.pcc.me.uk/lib/Object/IRSymtab.cpp#330

Apr 4 2019, 5:05 PM · Restricted Project

Apr 3 2019

steven_wu added a comment to D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

Apr 3 2019, 12:29 PM · Restricted Project
steven_wu created D60226: [ThinLTO] Fix ThinLTOCodegenerator to export llvm.used symbols.
Apr 3 2019, 12:29 PM · Restricted Project

Apr 1 2019

steven_wu added a comment to D59945: [ObjCMetadata] Add support for reading Objective-C metadata.

I am not convinced that there is sufficient abstraction here to handle multiple runtimes without significant code duplication. The Mach-O and LLVM IR parsers both appear to duplicate all knowledge of the runtime's data structures, so if you wanted to add a second runtime you'd then need four classes. I would expect to see two abstraction layers:

  1. Something allowing you to specify a data structure to examine that would load it from some binary (IR, Mach-O, ELF, whatever).
  2. Something built on top of this that then queries specific runtime metadata structures.

    In clang, we support two broad families of Objective-C runtime: Apple (Legacy and Modern, with version-specific things for macOS, iOS, and so on) and GNU (including GCC, GNUstep / WinObjC, and ObjFW). These are largely orthogonal to object file formats. For example, the GNUstep runtime is widely used with both ELF and PE/COFF binaries and can be used with Mach-O (though typically only for testing). If we wanted to add support for all of these, I suspect that we'd have a code explosion.
Apr 1 2019, 9:53 AM · Restricted Project

Mar 28 2019

steven_wu added a reviewer for D59945: [ObjCMetadata] Add support for reading Objective-C metadata: theraven.
Mar 28 2019, 10:07 AM · Restricted Project
steven_wu created D59945: [ObjCMetadata] Add support for reading Objective-C metadata.
Mar 28 2019, 10:02 AM · Restricted Project

Mar 22 2019

steven_wu added a comment to D59709: [ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible.

Thanks for explanation. Then the current approach in this patch LGTM. Thanks!

Mar 22 2019, 3:55 PM · Restricted Project
steven_wu added a comment to D59709: [ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Interesting, I made that change! But I somehow remembered I got pushed back because of ELF semantics and end up implementing that in ld64 instead. Let me dig up some context and get back to you.

Are you thinking of https://reviews.llvm.org/D43361?

Mar 22 2019, 2:56 PM · Restricted Project
steven_wu added a comment to D59709: [ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Mar 22 2019, 2:04 PM · Restricted Project
steven_wu added a comment to D59709: [ThinLTO] Auto-hide prevailing linkonce_odr only when all copies eligible.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

Mar 22 2019, 1:29 PM · Restricted Project

Mar 21 2019

steven_wu committed rG5a593547602b: [Object] Fix reading objects created with -fembed-bitcode-marker (authored by steven_wu).
[Object] Fix reading objects created with -fembed-bitcode-marker
Mar 21 2019, 2:02 PM
steven_wu committed rL356718: [Object] Fix reading objects created with -fembed-bitcode-marker.
[Object] Fix reading objects created with -fembed-bitcode-marker
Mar 21 2019, 2:00 PM
steven_wu closed D44373: Fix reading objects created with -fembed-bitcode-marker..
Mar 21 2019, 2:00 PM · Restricted Project
steven_wu added a comment to D44373: Fix reading objects created with -fembed-bitcode-marker..

Thanks for fixing this. LGTM.

Looks like this never actually got committed?

Mar 21 2019, 1:50 PM · Restricted Project

Mar 7 2019

steven_wu committed rGed9822928626: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic (authored by steven_wu).
[Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic
Mar 7 2019, 9:28 PM
steven_wu committed rL355663: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic.
[Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic
Mar 7 2019, 9:27 PM
steven_wu closed D59112: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic.
Mar 7 2019, 9:27 PM · Restricted Project
steven_wu created D59112: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic.
Mar 7 2019, 2:50 PM · Restricted Project

Feb 13 2019

steven_wu accepted D58122: Restore Check for Unreachable Exit Block in -Winfinite-recursion.

LGTM

Feb 13 2019, 9:18 AM · Restricted Project, Restricted Project

Feb 12 2019

steven_wu accepted D58071: Fix auto-upgrade for the new parameter to llvm.objectsize.

LGTM.

Feb 12 2019, 1:38 PM · Restricted Project

Feb 11 2019

steven_wu added a comment to D58071: Fix auto-upgrade for the new parameter to llvm.objectsize.

I talked to Erik offline. I think the upgrade tests should run on .bc file rather than text format.

Feb 11 2019, 3:38 PM · Restricted Project
steven_wu accepted D57991: [Driver][Darwin] Emit an error when using -pg on OS without support for it..

LGTM with a suggestion to make code cleaner.

Feb 11 2019, 11:02 AM · Restricted Project

Feb 7 2019

Herald added a project to D43737: Improve -Winfinite-recursion: Restricted Project.

Sorry for following up late on the patch. Removing the reachability testing for the exit block causes false positive for infinite loop cases like this:

Feb 7 2019, 11:34 AM · Restricted Project

Jan 29 2019

steven_wu committed rC352537: Fix the tests from r350970.
Fix the tests from r350970
Jan 29 2019, 12:13 PM
steven_wu committed rL352537: Fix the tests from r350970.
Fix the tests from r350970
Jan 29 2019, 12:13 PM

Jan 17 2019

steven_wu accepted D56805: mac: Correctly disable tools/lto tests when building with LLVM_ENABLE_PIC=OFF.

Not sure about gn part and if that has any impact. Otherwise, LGTM.

Jan 17 2019, 3:45 PM

Jan 11 2019

steven_wu committed rL350970: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.
[Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO
Jan 11 2019, 1:20 PM
steven_wu committed rC350970: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.
[Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO
Jan 11 2019, 1:20 PM
steven_wu closed D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.
Jan 11 2019, 1:20 PM
steven_wu updated the diff for D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.

Fix the comment

Jan 11 2019, 11:09 AM
steven_wu updated the diff for D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.

I was planning to add a test but I am not sure how to check the file type of temporary files.

Jan 11 2019, 10:55 AM
steven_wu created D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.
Jan 11 2019, 9:33 AM

Jan 7 2019

steven_wu added a comment to D53945: [TextAPI] TBD Reader/Writer.

Out of interest, why aren't most of the tests for this lit tests? The usual llvm way of testing stuff is to write a small llvm tool using your library (which will be useful outside of a testing context too) and then use that to write lit tests.

Looks like this is now happening (rL350341). Are most of the unit tests going to move to lit tests now?

Jan 7 2019, 10:05 AM · Restricted Project

Dec 17 2018

steven_wu added a comment to D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API.

Thanks for the clarifications. Would it be possible to utilize ThinBackendProc for this instead of a new CodeGenManager class? I.e. make a new derived version that does the index file write and spawns the local processes? The advantage is that it would start converging the implementations. And I think this could aid in refactoring suggested below to avoid duplication. Another advantage is that both LTO API's would have access to all backend implementations (in process, write indexes and exit, write indexes and use local processes, etc).

Dec 17 2018, 11:03 AM
steven_wu added inline comments to D55673: [darwin] parse the SDK settings from SDKSettings.json if it exists and pass in the -target-sdk-version to the compiler and backend.
Dec 17 2018, 10:38 AM
steven_wu accepted D55673: [darwin] parse the SDK settings from SDKSettings.json if it exists and pass in the -target-sdk-version to the compiler and backend.

Other than a small design choice commented inline, LGTM.

Dec 17 2018, 10:18 AM

Dec 13 2018

steven_wu added a comment to D55673: [darwin] parse the SDK settings from SDKSettings.json if it exists and pass in the -target-sdk-version to the compiler and backend.

See comments inline.

Dec 13 2018, 2:35 PM

Dec 12 2018

steven_wu accepted D55612: [macho] save the SDK version stored in module metadata into the version min and build version load commands in the object file.

I think I missed something before. LGTM now!

Dec 12 2018, 3:58 PM
steven_wu added a comment to D55612: [macho] save the SDK version stored in module metadata into the version min and build version load commands in the object file.

Don't we need IR support for this as well? sdk version is not in the triple so it is going to get lost when building from IR. Maybe use a metadata node?

I provide a way to set and retrieve the SDK version from the IR using the "SDK Version" metadata. Do I need something more than that? Clang can set it and we are already getting it in this patch.

Dec 12 2018, 3:30 PM
steven_wu added a comment to D55612: [macho] save the SDK version stored in module metadata into the version min and build version load commands in the object file.

Don't we need IR support for this as well? sdk version is not in the triple so it is going to get lost when building from IR. Maybe use a metadata node?

Dec 12 2018, 2:00 PM
steven_wu added a comment to D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API.

Thanks for taking a look. This patch is adding the customization points for thinLTO legacy API, which the code generator constructs clang invocations to do code generation. There is no dependency on any build system here and it only has a prove of concept codegen manager which invokes clang directly and collect the result back. You can replace this codegen manager with any protocol that is needed to talk to build system to run clang codegen.
XPC is the way to send information between process on Darwin, which is probably what we are going to use to talk to build system. If interested, I can post a patch which have example how to construct XPC communications, but there isn’t a build system you can use to listen on the other side to run the job yet.
When I say there are no code change for linker, I really mean there is no need to change a single line of code (maybe we need to add an API to select codegen manager in the future). ld64 really has a different approach using C API, which it tries to map the object file output back to the bitcode it gets as input. Terminating and relaunching the linker might has unexpected semantic changes for LTO. In the long run, maybe ld64 needs to design a new set of APIs to use the new C++ APIs but this is out of scope of this patch.

Dec 12 2018, 11:57 AM
steven_wu added a comment to D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API.

Ping. Is there any comments of implementing C API in this approach?

Dec 12 2018, 11:16 AM
steven_wu committed rL348943: [Driver] Add support for -fembed-bitcode for assembly file.
[Driver] Add support for -fembed-bitcode for assembly file
Dec 12 2018, 9:33 AM
steven_wu committed rC348943: [Driver] Add support for -fembed-bitcode for assembly file.
[Driver] Add support for -fembed-bitcode for assembly file
Dec 12 2018, 9:33 AM
steven_wu closed D55525: [Driver] Add support for -fembed-bitcode for assembly file.
Dec 12 2018, 9:33 AM

Dec 11 2018

steven_wu added a comment to D55525: [Driver] Add support for -fembed-bitcode for assembly file.

This really feels odd. Why not expect that the developer will add the content themselves? I'm not sure I understand the motivation for this change.

Dec 11 2018, 4:37 PM

Dec 10 2018

steven_wu created D55525: [Driver] Add support for -fembed-bitcode for assembly file.
Dec 10 2018, 11:04 AM
steven_wu abandoned D21230: Do not embed all the cc1 options in bitcode commandline.

This is upstreamed by Saleem already

Dec 10 2018, 11:04 AM

Nov 29 2018

steven_wu created D55080: [ThinLTO] Out-of-process CodeGenerator for legacy C API.
Nov 29 2018, 2:56 PM

Nov 26 2018

steven_wu accepted D54635: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.

I did some experiment and put some more thought into this patch. Not hashing PreservedSymbol passed to the C API is fine because the reason you mentioned but freestanding/no-builtin bits stills need to be handled separately in current status. That should be a different discussion so this patch is LGTM.

Nov 26 2018, 10:06 AM

Nov 16 2018

steven_wu added a comment to D54635: [ThinLTO] Consolidate cache key computation between new/old LTO APIs.

I wonder freestanding is still an issue after r316819. There is a function attribute for that now so it should be respected if the module is built with -ffreestanding and it should create different hash. I will do some digging to see if I can find the original report and how they setup the build. @mehdi_amini, do you remember the original problem?

Nov 16 2018, 11:03 AM

Nov 14 2018

steven_wu accepted D54542: Remove unused getMDNodeFwdRefOrNull interfaces (NFC).

Thanks!

Nov 14 2018, 1:54 PM
steven_wu added a comment to D53596: [ThinLTO] Fix a crash in lazy loading of Metadata.

Steven - you had a suggestion around callers to getMDNodeFwdRefOrNull, but after this change there will be no more callers to that. So should I go ahead and submit this one, then remove that interface completely in a follow up? Or do it in this patch?

Nov 14 2018, 12:11 PM
steven_wu added a comment to D53596: [ThinLTO] Fix a crash in lazy loading of Metadata.

My thought is that this patch should make bitcode reader more robust when lazy loading, which is always a good thing. If this is a performance regression, the regression is coming from how debug information is generated. If there are no such forward-ref in the metadata, there is no slowdown with this patch. We can investigate performance regression post commit if needed.

Nov 14 2018, 11:17 AM

Nov 13 2018

steven_wu added a comment to D49362: [ThinLTO] Internalize read only globals.

I reverted this in r346768.

Nov 13 2018, 9:53 AM