In D88309#2295403, @w2yehia wrote:

@steven_wu what about the alternative solution where we make libLTO query the --filetype command line option. This will not require adding a new function to the API, but it does change the behavior of existing functions (lto_codegen_write_merged_modules, lto_codegen_compile, lto_codegen_optimize, lto_codegen_compile_optimized, lto_codegen_compile_to_file)

diff --git a/llvm/tools/lto/lto.cpp b/llvm/tools/lto/lto.cpp
index b680714..8251f7b 100644
--- a/llvm/tools/lto/lto.cpp
+++ b/llvm/tools/lto/lto.cpp
@@ -157,20 +157,23 @@
 
 // Convert the subtarget features into a string to pass to LTOCodeGenerator.
 static void lto_add_attrs(lto_code_gen_t cg) {
   LTOCodeGenerator *CG = unwrap(cg);
   auto MAttrs = codegen::getMAttrs();
   if (!MAttrs.empty()) {
     std::string attrs = join(MAttrs, ",");
     CG->setAttr(attrs);
   }
 
+  if (auto FT = codegen::getExplicitFileType())
+    CG->setFileType(FT.getValue());
+
   if (OptLevel < '0' || OptLevel > '3')
     report_fatal_error("Optimization level must be between 0 and 3");
   CG->setOptLevel(OptLevel - '0');
   CG->setFreestanding(EnableFreestanding);
 }

In that case, please increment LTO_API_VERSION. If you think the API needs more documentation, please add to docs/LinkTimeOptimization.rst

In D88309#2295281, @hubert.reinterpretcast wrote:

In D88309#2295142, @steven_wu wrote:

• If this is just a temporary workaround, I would resist make this change to bake in this API that will not be used in the future.

This seems to be conflating the immediate reason with the more general value of having such an interface. The interface that @tejohnson's earlier comment mentions presumably exists because there is value in having such an interface.

Note libLTO API needs to be stable so:

• If you need to add API, you need to bump API version, etc. Please refer to other commits that add APIs to see what needs to be done.
• If this is just a temporary workaround, I would resist make this change to bake in this API that will not be used in the future.

Can you clarify your motivation for this? libLTO is used as an interface for linker and linkers, in general, do not understand assembly files.

LGTM

Ok, I guess we are on the same page. The idea sounds fine to me.

In D88114#2288737, @mtrofin wrote:

In D88114#2288732, @steven_wu wrote:

I am not sure what exactly is expected here. What is your definition for pre-optimized bitcode and how your test case ensures that? Can you explain a bit more for context?

Pre-optimized meaning before the llvm optimization pipeline is called. That's the current implementation, and the test explicitly checks that the inlining of bar into foo doesn't happen.

I could add an "alwaysinline" to bar, to further stress that.

I am not sure what exactly is expected here. What is your definition for pre-optimized bitcode and how your test case ensures that? Can you explain a bit more for context?

LGTM. Thanks.

SGTM.

seems fine. The only worry is that if you have a stack that is close to the stack limit and the unwinder might just blow the stack away so it might have bincompat concerns.

In D84803#2245819, @dmajor wrote:

Our builds are back to green with that change. I agree that the root cause is likely elsewhere, but getting to the bottom of it would unfortunately take more time than I am able to commit.

In D86767#2245775, @rprichard wrote:

In D86767#2244767, @steven_wu wrote:

Sorry for the inconvenience that libunwind is not really buildable for armv7 from iOS SDK. You can build a single threaded version without referring to those interfaces. If this is really troublesome for you, feel free to file a bug report and we can see what can be done here.

Thanks. I think I'm OK as-is.

This patch is technically not correct from a bigger context. I think, for example, watch armv7k doesn't default to sjlj exception model but it needs to build sjlj model to support some clients.

That makes sense. It looks like passing -arch armv7k to clang is enough to switch it to using DWARF and compact unwinding rather than sjlj. I also see a comment in ToolChains/Darwin.cpp:
// Only watchOS uses the new DWARF/Compact unwinding method.

It seems that Darwin/armv7k libunwind doesn't build because config.h enables _LIBUNWIND_SUPPORT_COMPACT_UNWIND, but there is no 32-bit ARM compact unwinder. It looks like setting the flag is the right thing to do, though -- maybe the rest of the code is internal to Apple.

I'll revise or abandon this patch. My main goal was to ensure that if _LIBUNWIND_USE_DL_ITERATE_PHDR was defined, then either of _LIBUNWIND_ARM_EHABI or _LIBUNWIND_SUPPORT_DWARF_UNWIND would also be defined.

In D86847#2248354, @MaskRay wrote:

In D86847#2248353, @steven_wu wrote:

I am a bit worry that linker might concatenate bitcode file with padding to achieve alignment requirement, etc. I guess you can create a termination block to mark the end but it is hard to seek the next start.

We can set the section alignment to 1 to avoid padding: (I rushed a bit, sorry: 6ae7b403c3e1aebcb825d3dd4777d3c1149d6d67)

I am a bit worry that linker might concatenate bitcode file with padding to achieve alignment requirement, etc. I guess you can create a termination block to mark the end but it is hard to seek the next start.

Thanks. LGTM.

In D84803#2245283, @dmajor wrote:

Yeah, I wish I could give a nice test case, but it's a huge build and the affected configuration needs a lot of setup so I can't even replicate it locally at the moment.

If you have proposed patches I could send them off for a dry run relatively easily though.

In D84803#2244918, @dmajor wrote:

Hi, we're seeing an assertion failure in one of the Firefox LTO bots:

char llvm::StringRef::operator[](size_t) const: Assertion `Index < Length && "Invalid index!"' failed.

I'm still working on finishing the bisection but I have a hunch that it may be this change. Unfortunately I can't seem to get a backtrace out of this particular bot.

As a guess, could it be the operator[] in getGlobalIdentifier at https://github.com/llvm/llvm-project/blob/4cb016cd2d8467c572b2e5c5d34f376ee79e4ac1/llvm/lib/IR/Globals.cpp#L140 ? It looks like there was previously a check for Name.size() > 0 that isn't checked anymore.

Sorry for the inconvenience that libunwind is not really buildable for armv7 from iOS SDK. You can build a single threaded version without referring to those interfaces. If this is really troublesome for you, feel free to file a bug report and we can see what can be done here.

ping

ping

LGTM

Also added a followup that can potentially deal with the conflict between "_$NAME" and "\01_$NAME". Not sure if that is going to affect some other platforms or not so I put them in a separate review: https://reviews.llvm.org/D85564

Address review feedback.

LGTM

Change how the GUID is computed in ThinLTOCodeGenerator.

In D84803#2185566, @mehdi_amini wrote:

So getGlobalIdentifier removes the leading \01 because it only indicates that the symbol is already mangled, but on the other hand does not apply the platform mangling when there isn't a \01? It seems inconsistent isn't it? Could getGlobalIdentifier mangle the symbol when there isn't the leading \01?

In D84803#2185095, @tejohnson wrote:

In D84803#2184985, @steven_wu wrote:

In D84803#2184976, @tejohnson wrote:

Not very familiar with MachO. Can you clarify the issue - is it the case that some modules will use the mangled names and some won't? Otherwise it seems like it could use either all mangled or all unmangled.

The issue is that for macho, for a symbol in the object file (symbol table seen by the linker), let's say "_symbol". It can be either. "symbol" or "\01_symbol" in the bitcode file. When user/linker says it want to preserve "_symbol", you don't know what the GUID for that symbol is because it can be both.

I see - so this is an issue with the way the user has specified the name through some kind of option to say it needs to be preserved.

In D84803#2184976, @tejohnson wrote:

Not very familiar with MachO. Can you clarify the issue - is it the case that some modules will use the mangled names and some won't? Otherwise it seems like it could use either all mangled or all unmangled.

This is basically a fundamental issues in GUID for machO format, which I am not sure if we can fix the underlying issue. That requires computing GUID based on mangled name, which can be more expensive and I am not sure if it will break the module summary for compatibility if we switch algorithm. So I fixed the export/preserve list only because that is the only thing I can think of that will surface as a bug. Otherwise, the mismatch GUID will just be a missed optimization opportunity.

I have seen that and was wondering why. Thanks for figuring that out!

LGTM

rebase the patch and ping.

Address review feedback

Address review feedback and also add a testcase to make sure good branch
weights are not stripped.

Ping

Use Option::matches instead.

Add comments in the tests.

LGTM

LGTM.

Agree this is ugly but the clean up looks fine. Not sure how to write a test as well but I can see the @ file path is triggered correctly.

LGTM

Not sure if it makes more sense to break the patch into two commits:

• config.guess change is for building the correct host triple on apple silicon machine without explicitly specify it.
• the driver change is for better default on Apple silicon Mac.

LGTM

LGTM

It looks good in general. I would prefer to have two separate tests, one for upgrade, one for ir-linking.

In D75617#1908254, @evgeny777 wrote:

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

It is for debugging. Not WPD itself but rather a devirtualized binary. See comment above

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

Thanks. LGTM. I will let @arphaman to approve this.

The 0x40 bit is actually OBJC_IMAGE_HAS_CATEGORY_CLASS_PROPERTIES, which probably can differ from translation unit to translation unit even within ObjC. Some of the other bits should not be different. That is why I say you can break the bits down even more if needed.

Clang seems to set it unconditionally. Presumably it just controls whether category descriptors have a class-properties field; offhand, I don't know why that would need to be set globally.

I agree with all of this except that I'm not sure there's any situation where ObjC and Swift translation units should be disagreeing about the ObjC-specific parts of the OBJC_IMAGE_INFO.

In D71219#1892292, @manmanren wrote:

We should add a new module flag (with type Error) that contains Major and Minor and make sure "Garbage Collection" value is the same for Swift as for ObjC.

In the backend, we will reconstruct IMAGE_INFO from the new module flags and make sure the value stays the same with and without this patch.

IMAGE_INFO will be different for Swift vs ObjC.

Should be question mark! Meant for clarification from @steven_wu and @rjmccall. Should IMAGE_INFO include the value from the new module flag?

In D74784#1891750, @dmajor wrote:

@steven_wu, ping, could you clarify about the tests please?

In D71219#1890444, @jinlin wrote:

In D71219#1791936, @steven_wu wrote:

1. I will add a llvm-link tests for combining objc and swift bitcode.
2. What do you mean by "codegen tests for correct value in the object file"? There is no "object file" in the picture. Are you asking for a test that checks the value in the output of llvm-link?

I think the decision was to break apart the current "Objective-C Garbage Collection" metadata. You need codegen test to test that OBJC_IMAGE_INFO is generated correctly after breaking apart the value.

I have question regarding this. Let's assume we have one assembly file from Objective-C and another one from Swift.

Objective-C assembly:
L_OBJC_IMAGE_INFO:
.long 0
.long 64

Swift assembly:

L_OBJC_IMAGE_INFO:
.long 0
.long 83953472

Let's assume that the llvm-link can link Swift bitcode and Objective-C bit successfully and the llc can generate the assembly for the merged output bitcode.

In the final assembly file, what value do you expect for L_OBJC_IMAGE_INFO?

In D74994#1889980, @sbc100 wrote:

In D74994#1889957, @steven_wu wrote:

I don't feel like this is necessary and creating a subdirectory without creating a new library feels weird. I agree the layout is a bit weird for new comer to LTO library but it is a relatively small hurdle. I don't exactly feel like I want to pay the cost of making git blame a bit harder.

Any other reason for changing the layout?

Just what I said in the comment. I'm not wedded to this change. Will drop it if others don't think it makes sense.

Out of interest do you know if there are plans to remove the legacy code one day?

I don't feel like this is necessary and creating a subdirectory without creating a new library feels weird. I agree the layout is a bit weird for new comer to LTO library but it is a relatively small hurdle. I don't exactly feel like I want to pay the cost of making git blame a bit harder.

ping

I forgot if there is reason to use the option by default at all time (I did ask that in the previous review but Alex might have given more context offline).