Run ObjCContractPass in Default Codegen Pipeline

[NuriAmari]

Prior to this patch, when using -fthinlto-index= the ObjCARCContractPass isn't run prior to CodeGen, and instruction selection fails on IR containing arc intrinsics.

The pass would normally be added here:

llvm-project/clang/lib/CodeGen/BackendUtil.cpp

Line 1352 in c00e012

EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M, VFS);

Since we are using an index file, we return early above after runThinLTOBackend:

llvm-project/clang/lib/CodeGen/BackendUtil.cpp

Lines 1333 to 1339 in c00e012

	if (CombinedIndex) {
	if (!CombinedIndex->skipModuleByDistributedBackend()) {
	runThinLTOBackend(Diags, CombinedIndex.get(), M, HeaderOpts, CGOpts,
	TOpts, LOpts, std::move(OS), CGOpts.SampleProfileFile,
	CGOpts.ProfileRemappingFile, Action);
	return;
	}

[llvmbot]

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-backend-powerpc
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-transforms
@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lto
@llvm/pr-subscribers-lld-macho
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Nuri Amari (NuriAmari)

Changes

Prior to this patch, when using -fthinlto-index= the ObjCARCContractPass isn't run prior to CodeGen, and instruction selection fails on IR containing arc intrinsics.

The pass would normally be added here:

llvm-project/clang/lib/CodeGen/BackendUtil.cpp

Line 1352 in c00e012

EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M, VFS);

Since because we are using an index file, we return early above after runThinLTOBackend:

llvm-project/clang/lib/CodeGen/BackendUtil.cpp

Lines 1333 to 1339 in c00e012

	if (CombinedIndex) {
	if (!CombinedIndex->skipModuleByDistributedBackend()) {
	runThinLTOBackend(Diags, CombinedIndex.get(), M, HeaderOpts, CGOpts,
	TOpts, LOpts, std::move(OS), CGOpts.SampleProfileFile,
	CGOpts.ProfileRemappingFile, Action);
	return;
	}

---

Full diff: https://github.com/llvm/llvm-project/pull/92331.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/BackendUtil.cpp (+3)
• (added) clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll (+19)

diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 90985c08fe7f8..03dd1df281d80 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -1289,6 +1289,9 @@ static void runThinLTOBackend(
     };
     break;
   default:
+    Conf.PreCodeGenPassesHook = [](legacy::PassManager &Pm) {
+      Pm.add(createObjCARCContractPass());
+    };
     Conf.CGFileType = getCodeGenFileType(Action);
     break;
   }
diff --git a/clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll b/clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll
new file mode 100644
index 0000000000000..7d0247555b5c8
--- /dev/null
+++ b/clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll
@@ -0,0 +1,19 @@
+; RUN: opt -thinlto-bc -o %t.o %s
+
+; RUN: llvm-lto2 run -thinlto-distributed-indexes %t.o \
+; RUN:   -o %t2.index \
+; RUN:   -r=%t.o,_use_arc,px
+
+; RUN: %clang_cc1 -triple x86_64-apple-darwin \
+; RUN:   -emit-obj -fthinlto-index=%t.o.thinlto.bc \
+; RUN:   -o %t.native.o -x ir %t.o
+
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-darwin"
+
+define void @use_arc(ptr %a, ptr %b) {
+  call void (...) @llvm.objc.clang.arc.use(i8* %a, i8* %b) nounwind
+  ret void
+}
+
+declare void @llvm.objc.clang.arc.use(...) nounwind

[fhahn]

Shouldn't this be added to the LTO code generator? In libLTO (used by Apple's linker) it is added here llvm/lib/LTO/ThinLTOCodeGenerator.cpp

[kyulee-com]

I think ObjCARCContractPass is ideally suited for MachO rather than ELF. There might be a consideration to add it conditionally, however, this could be excessive. Like the (full)LTO pass -- https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTOCodeGenerator.cpp#L141, I think this approach results in a no-op for ELF and it should be okay.

[NuriAmari]

Shouldn't this be added to the LTO code generator? In libLTO (used by Apple's linker) it is added here llvm/lib/LTO/ThinLTOCodeGenerator.cpp

Presumably because the pass is likely not useful unless targeting MachO, LLD does this via configuration hook: https://reviews.llvm.org/D94547. I'm already doing it unconditionally, so what do you think about adding the pass here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTOBackend.cpp#L417, and deleting this: https://github.com/llvm/llvm-project/blob/main/lld/MachO/LTO.cpp#L51-L53 ?

[NuriAmari]

Shouldn't this be added to the LTO code generator? In libLTO (used by Apple's linker) it is added here llvm/lib/LTO/ThinLTOCodeGenerator.cpp

Presumably because the pass is likely not useful unless targeting MachO, LLD does this via configuration hook: https://reviews.llvm.org/D94547. I'm already doing it unconditionally, so what do you think about adding the pass here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTOBackend.cpp#L417, and deleting this: https://github.com/llvm/llvm-project/blob/main/lld/MachO/LTO.cpp#L51-L53 ?

I've made this change

[kyulee-com]

Because you now add it to codegen unconditionally, do you also need to delete https://github.com/llvm/llvm-project/blob/main/llvm/lib/LTO/LTOCodeGenerator.cpp#L141?

[aeubanks]

it seems like this should just be in the default codegen pipeline? you'd need to change the pass to bail out early if there are no relevant intrinsics (by checking if the module contains the intrinsic declaration) to not affect compile times

[cachemeifyoucan]

it seems like this should just be in the default codegen pipeline? you'd need to change the pass to bail out early if there are no relevant intrinsics (by checking if the module contains the intrinsic declaration) to not affect compile times

Maybe that is a good idea since the check is already there and the check is quite cheap (just check the existence of ARC intrinsics).

I don't know about how lld is configured, but the concept of the change SGTM. The pass is missing probably because we don't link with distributed thinLTO that uses index-only option.

[NuriAmari]

it seems like this should just be in the default codegen pipeline? you'd need to change the pass to bail out early if there are no relevant intrinsics (by checking if the module contains the intrinsic declaration) to not affect compile times

I've done this, and removed the other places the pass is added currently. Let me know if the location I've added it looks reasonable. Thanks.

Maybe that is a good idea since the check is already there and the check is quite cheap (just check the existence of ARC intrinsics).

@cachemeifyoucan I couldn't find this check you're alluding to, so I added my own. If the check does exist, could you point me to it? If not, does mine look reasonable? Thanks.

[cachemeifyoucan]

I couldn't find this check you're alluding to, so I added my own. If the check does exist, could you point me to it? If not, does mine look reasonable? Thanks.

I was answering from my memory, now I have to check. We have this function ModuleHasARC which is used in other ARC optimization passes. It used to be in arc-contract pass too but it seems to be dropped by https://reviews.llvm.org/D92808

It is a good idea to added it back. Maybe by reusing the same function after double check the context of the old patch. @ahatanaka might have idea if there is a reason to drop the guard.

[NuriAmari]

I've added the check back in, and reverted my custom check. There is too much going on in https://reviews.llvm.org/D92808 for me to quickly understand everything, but I don't see an obvious reason the check needed to be removed. @ahatanaka Please let me know if I'm missing something.

[nikic]

Reverted due to major compile-time regressions for unoptimized builds, see https://llvm-compile-time-tracker.com/compare.php?from=910292c3ac2ebe43cdbc90223c6c9702128316db&to=8cc8e5d6c6ac9bfc888f3449f7e424678deae8c2&stat=instructions:u.

[nikic]

Presumably the extra analysis passes are the reason for the compile-time regressions. While you may be skipping the execution of the pass itself, you are not skipping the execution of the analyses.

[aeubanks]

this is where having the new pass manager for codegen would be nice

[NuriAmari]

I suspected this might be an issue. Is it possible to "lazily" request analysis results based on some logic in the pass itself? If not, maybe we can't enable this pass by default after all.