[ThinLTO] Skip BlockAddresses while replacing uses in function import

[dmikulin]

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=36219: [ThinLTO] link asserts 'BA && "Refcount and block address map disagree!"'
This is similar to https://bugs.llvm.org/show_bug.cgi?id=35201: CFI: Assert triggered by LowerTypeTests

[tejohnson]

I'd like @pcc to comment. I noticed there are a number of other places in this file where we will still be calling replaceAllUsesWith on a Function. How do we know which should use which method?

[pcc]

In D43027#1000808, @tejohnson wrote:

I'd like @pcc to comment. I noticed there are a number of other places in this file where we will still be calling replaceAllUsesWith on a Function. How do we know which should use which method?

We need to use replaceUsesExceptBlockAddr if the function that we are replacing is a function definition. As far as I can tell, in all other cases we are replacing function declarations.

llvm/test/ThinLTO/X86/blockaddr-import.ll

2 ↗

(On Diff #133240)

Can this be written as an IR-level test of the pass? See for example llvm/test/Transforms/LowerTypeTests/import-icall.ll.

[dmikulin]

Re-wrote the test.

[pcc]

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll

2

I don't think you need -O2 here.

30

Please use FileCheck to check that the blockaddress refers to the correct function.

[dmikulin]

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll

2

I do not need the whole O2 pipeline, but the verifier relies on some analysis passes to set address-taken flags to assert. I changed it to -O1.

30

Added a check to make sure blockaddress refers to the new function, m.cfi.

[dmikulin]

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll

2

Looks like having the check to make sure the new function is used in blockaddress might be sufficient. I'll take out the -O option.

[dmikulin]

Fixed the test.

[pcc]

LGTM

llvm/test/Transforms/LowerTypeTests/Inputs/blockaddr-import.yaml

2–6

I think you can remove this part.

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll

11

Is this function still necessary?

[pcc]

llvm/test/Transforms/LowerTypeTests/Inputs/blockaddr-import.yaml

2–6

It looks like this wasn't addressed.

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

if (F->isDeclarationForLinker() && !isDefinition) {
     // so that it is treated the same way as a function defined outside of the
     // LTO unit.
     return;
   }
 
   if (F->isWeakForLinker())
     replaceWeakDeclarationWithJumpTablePtr(F, FDecl);
   else
-    F->replaceAllUsesWith(FDecl);
+    F->replaceUsesExceptBlockAddr(FDecl);
 }
 
 void LowerTypeTestsModule::lowerTypeTestCalls(
     ArrayRef<Metadata *> TypeIds, Constant *CombinedGlobalAddr,
     const DenseMap<GlobalTypeMember *, uint64_t> &GlobalLayout) {
   CombinedGlobalAddr = ConstantExpr::getBitCast(CombinedGlobalAddr, Int8PtrTy);
 
   // For each type identifier in this disjoint set...

llvm/test/Transforms/LowerTypeTests/Inputs/blockaddr-import.yaml

+---
+TypeIdMap:
+  typeid1:
+    TTRes:
+      Kind:            AllOnes
+      SizeM1BitWidth:  7
+WithGlobalValueDeadStripping: true
+CfiFunctionDefs:
+  - l
+  - m
+CfiFunctionDecls:
+...

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll

+; RUN: opt -lowertypetests -lowertypetests-summary-action=import -lowertypetests-read-summary=%p/Inputs/blockaddr-import.yaml %s -S | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux"
+
+@d = common hidden local_unnamed_addr global void (...)* null, align 8
+
+declare i1 @llvm.type.test(i8*, metadata) #1
+declare !type !11 i32 @o(...)
+
+define hidden void @l() local_unnamed_addr #0 !type !3 {
+entry:
+  %0 = load void (...)*, void (...)** @d, align 8
+  %1 = bitcast void (...)* %0 to i8*
+  %2 = tail call i1 @llvm.type.test(i8* %1, metadata !"_ZTSFvE")
+  br i1 %2, label %cont, label %handler.cfi_check_fail
+
+handler.cfi_check_fail:
+  br label %cont
+
+cont:
+  ret void
+}
+
+define hidden void @m() #0 !type !3 {
+entry:
+  br label %n
+n:
+  %call = tail call i32 (i8*, ...) bitcast (i32 (...)* @o to i32 (i8*, ...)*)(i8* blockaddress(@m, %n)) #4
+; Make sure that blockaddress refers to the new function, m.cfi
+; CHECK: define hidden void @m.cfi()
+; CHECK: blockaddress(@m.cfi, %n)
+
+  ret void
+}
+
+!3 = !{i64 0, !"_ZTSFvE"}
+!11 = !{i64 0, !"_ZTSFiE"}