[ThinLTO] Fix missing call graph edges for calls with bitcasts.

[vsapsai]

Summary

Fixes PR34966 as added call graph edges allow to build correct module
imports/exports. With fixed imports/exports ThinLTO can see which
functions are used and keeps both static and non-static functions as
both of them are used.

rdar://problem/35099885

[vsapsai]

I'm not entirely confident in the test. It's not Clang output but manually assembled LLVM IR from different sources. So if you notice something strange, don't hesitate to point it out.

[mehdi_amini]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

Isn't it enough to just replace auto *CalledValue = CS.getCalledValue(); with auto *CalledValue = CS.getCalledValue()->stripPointerCastsNoFollowAliases(); ?

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

Not enough. CalledValue will be what we want (and hopefully it won't be NULL) but ImmutableCallSite CS will still use callee with pointer cast and CalledFunction will be NULL.

Another approach can be adding stripPointerCastsNoFollowAliases to ImmutableCallSite or CallSiteBase. Or we can introduce another type of call site and add it there. I don't know if stripping casts will be useful or harmful in other places, for this I rely on people who are more familiar with this code.

[mehdi_amini]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

It is a good point that there is likely some general goodness in looking through pointer cast in ImmutableCallSite and/or CallSiteBase.
I'd be interested to see if ninja check-all is passing after adding stripPointerCastsNoFollowAliases to CallSiteBase, and if not what the failures are.

[tejohnson]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

247 ↗

(On Diff #120527)

Do we need to do this if CalledFunction is non-NULL? I would think the above condition could be "if (CalledValue and !CalledFunction)".

251 ↗

(On Diff #120527)

Agree, this would be a good thing to try.

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

247 ↗

(On Diff #120527)

It works both with if (CalledValue) and with if (CalledValue and !CalledFunction). My goal is to strip pointer casts in all cases, unless the value is NULL. And the condition captures this intention. if (CalledValue and !CalledFunction) is more an implementation detail because we know CalledFunction to be of type Function* and casts are never of type Function*, so stripPointerCastsNoFollowAliases and non-NULL CalledFunction are mutually exclusive.

That was my reasoning, don't have strong opinion about it.

251 ↗

(On Diff #120527)

I've tried naive approach to add stripPointerCastsNoFollowAliases to CallSiteBase and got 544 test failures. Looks like it interferes with type checking, for instance Verifier/speculatable-callsite.ll (and many others) failed with

llvm/clang-build-release/bin/llvm-as: assembly parsed, but does not verify as correct!
Called function is not the same type as the call!
  %ret = call float bitcast (i32 ()* @speculatable to float ()*)() #0

Another popular problem is verifier complaining "Called function is not pointer to function type!" for code like

%fptrptr = getelementptr [1 x i8*], [1 x i8*]* %vtable, i32 0, i32 0
%fptr = load i8*, i8** %fptrptr
%fptr_casted = bitcast i8* %fptr to void (i8*)*
call void %fptr_casted(i8* %obj)

After adding stripPointerCastsNoFollowAliases only to ImmutableCallSite I got 7 failing tests, among them 3

Assertion failed: (isa<X>(Val) && "cast<Ty>() argument of incompatible type!"), function cast, file llvm-project/llvm/include/llvm/Support/Casting.h, line 255.

for

LLVM :: Transforms/Coroutines/ex5.ll
LLVM :: Transforms/Inline/cgscc-cycle.ll
LLVM :: Transforms/InstCombine/2007-05-18-CastFoldBug.ll

and 2

Assertion failed: (i < getNumArgOperands() && "Out of bounds!"), function getArgOperand, file llvm-project/llvm/include/llvm/IR/Instructions.h, line 1572.

for

LLVM :: Transforms/Inline/inline-musttail-varargs.ll
LLVM :: Transforms/Inline/noinline-recursive-fn.ll

My conclusion is that more aggressive stripPointerCastsNoFollowAliases usage gives bad results and isn't worth pursuing as there is code expecting casts to be present. What do you think?

[efriedma]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

LLVM IR optimizations generally treat a call whose callee is a bitcast as an indirect call. This is precisely because they want to assume the called function has the same signature as the call. I don't think this is worth changing.

The one place that does have special handling for calls of bitcasts is instcombine: it has some code to try to turn them into direct calls in simple cases.

Also, it's suspicious that you're doing this to try to fix a bug: unless you're trying to match some other code which is also calling stripPointerCastsNoFollowAliases() on the callees of calls, you're just hiding a real problem with function pointer handling.

[mehdi_amini]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

Is the inliner having a special handling for calls of bitcasts as well? If yes we should try to register this as a call, otherwise we could go with a reference.

[efriedma]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

No, the inliner has no special handling; it treats calls where the callee is a bitcast as indirect calls.

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

So how do you recommend to address this problem? I've tried to leave CalledFunction NULL and handle it in the branch for indirect calls. But IndirectCallPromotionAnalysis doesn't return any promotion candidates because call instruction has no MD_prof metadata. As the result, nothing is added to the call graph.

[efriedma]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

Well, in terms of fixing the PR34966 miscompile, this patch is essentially orthogonal. ThinLTO importing should generate correct code whether or not a function pointer is called directly.

In terms of trying to improve performance for pre-ANSI C code, I guess this patch is fine? It looks like adding the edge won't do any harm even if the rest of the optimizer continues to treat it as an indirect call.

[tejohnson]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

247 ↗

(On Diff #120527)

Personally I prefer changing to "if (CalledValue and !CalledFunction)". I think it is clearer because there is no chance of having a cast that needs stripping if we have CalledFunction!=null. If we have a CalledFunction, then we will add the appropriate edges.

251 ↗

(On Diff #120527)

Well, in terms of fixing the PR34966 miscompile, this patch is essentially orthogonal. ThinLTO importing should generate correct code whether or not a function pointer is called directly.

I'm not sure why you say this patch is orthogonal to the miscompile. We need these edges in the summary not only for importing, but also for things like liveness analysis and knowing what to promote as a result of other importing decisions. The fact that no edge got added here meant that we didn't realize the callee function was exported when it's caller was imported. As Mehdi points out, we could also add the callee to the reference edges for the caller, but by adding a call edge we are able to import it and if some other optimization can remove the bitcast or otherwise convert to a "direct" call we would be able to inline it.

[dexonsmith]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

I don't understand Eli's argument that this fix is unrelated to the miscompile. As Teresa said, ThinLTO needs an accurate call graph in order to correctly promote static functions. This does exactly that.

It's irrelevant (to the miscompile) whether the static function is inlinable.

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

247 ↗

(On Diff #120527)

OK, will change.

251 ↗

(On Diff #120527)

After more thinking and investigation I think there is more to this problem. It is still not clear why duplicate function name caused miscompile. If you change static function name - everything is fine, if you disable LTO - linker is perfectly happy with duplicate function names and doesn't mix them. This fix avoids the problem of duplicate names by allowing to promote static function and to rename it. But I haven't found yet why duplicate names aren't handled correctly with ThinLTO.

Another test I've done is renaming static function and checking module index. Imports/exports are still empty but the final binary is correct. That makes me think that there is another bug not related to module index.

[dexonsmith]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

If you change static function name - everything is fine

Okay; I agree that's strange.

if you disable LTO - linker is perfectly happy with duplicate function names and doesn't mix them

Sure, of course not. The linker has been getting this right for decades.

[steven_wu]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

If linking the object works, I don't think LTO mess up the visibility.

There is a stage in ld64 called AtomSyncer which maps the information from llvm-ir to generated object file. It is likely that the undef is sync to the static version in error.

[tejohnson]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

I also agree that is odd that changing the static name would allow things to work.

Normally the there is no issue as the compiler/linker know to resolve to the static function in that file. In the case in the PR, with ext() imported and without the static f() being promoted/renamed, it presumably links with the wrong version since that is the only version available for the use in the imported copy of ext(). So the initial analysis and fix make sense to me.

But it would be good to understand why it works if the static function name is changed.

[efriedma]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

In general, if the callee is a ConstantExpr, you can't determine the function it will call. Consider, e.g.:

define void @h() {
entry:
  tail call void select (i1 icmp eq (i32* @a, i32* null), void ()* @f, void ()* @g)()
  ret void
}

I'm not that familiar with ThinLTO, so I'm not sure how a "reference edge" is different from a "call edge". But in general you need to walk the ConstantExpr to find which globals it refers to.

[mehdi_amini]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

, if you disable LTO - linker is perfectly happy with duplicate function names and doesn't mix them.

There is no real "duplicate name": the naming is scoped and a static function isn't in the global scope, its name is meaningless.
This is also why I mentioned earlier that I'd like to see the importer fixed: it shouldn't allow to import a function with a reference to an internal one and have the reference being "remapped" to a global function in the current file (I assume that the issue is at the LLVM level, not the linker...). There should be a fatal error when the client of the importer tries to do that.

I don't understand how renaming the function solves the problem though: there is something fishy. I'd be interested to get the intermediate files (pass -save-temps to the linker) to understand what's going on.

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

OK, I believe I've found a place in ld64 where we adjust fixups and associate them with other atoms based on name only, without checking linkage visibility. It is in AtomSyncer as Steven has helpfully mentioned. That issue is investigated separately.

With -save-temps I tried to link intermediate files manually, like ld -o a-manual.out a.out.0.thinlto.o a.out.1.thinlto.o a.out.2.thinlto.o and resulting binary was correct. So I make the conclusion that ThinLTO object file generation is working correctly, the problem is in linker integration with ThinLTO.

Back to the current patch. Is there value in it in order to improve performance for pre-ANSI C code?

[mehdi_amini]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

@efriedma : usually a reference edge is what we have when we load from a global variable, or take the address of a symbol (variable or function). IIRC it is always OK to add extra edges (calls and references) in the graph.

Call edges are used to compute function importing, while reference edges are used for things like dead-stripping.

As Teresa mentioned, speculatively inserting a call edge means we are likely to import the function and get it inlined in case the indirect call becomes direct (or if indirect call speculation happens).

By the way with PGO and value-profiling we speculate some call edges as well I think.

@vsapsai thanks for checking ld64! I think we should still move on with this patch, but I'd like to hear from Teresa.

[tejohnson]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

Hi All, a few comments/questions.

First - can someone clarify what the issue is with pre-Ansi C code? Why is this only an issue there?

It seems to me like we would have an issue here regardless - without this patch we are missing an edge in the call/reference graph, right? That will mess up various analyses.

@efriedma As Mehdi mentioned we do also add reference edges. You can see the code here:
http://llvm-cs.pcc.me.uk/lib/Analysis/ModuleSummaryAnalysis.cpp#89
Essentially we do walk all the operands of each instruction looking for any type of GV references. We add them to the reference edge unless the instruction is an ImmutableCallSite and the GV isCallee of that callsite. The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge. But then due to the bitcast, getCalledFunction was null, so we didn't add the call edge either.

I would suggest the following to make sure we don't have any lurking issues like this - we should add an assert in the indirect call handling case further down - rather than skipping direct calls, if we have a Constant CalledValue we should assert since it means we didn't add an edge to represent that call anywhere. I.e. we wouldn't have added a reference edge, because isCallee would have returned true for that GV, and we obviously didn't add a direct edge since we went into the else clause to handle the indirect case (and we have an earlier continue to skip inline asm calls). I'm ok with adding this assert as a follow-on patch though, because I don't want to block this fix if that assert flushes out additional issues.

[vsapsai]

• Address review comment: be more conservative in stripPointerCastsNoFollowAliases().
• Assert that we don't have direct calls in branch for indirect calls instead of simply skipping direct calls.

Added assertion didn't cause any test failures in ninja check-all when
running locally.

[vsapsai]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

251 ↗

(On Diff #120527)

First - can someone clarify what the issue is with pre-Ansi C code? Why is this only an issue there?

I don't know this area well, so correct me if I am wrong. The crucial part is distinction between int ext(); and int ext(void);. In pre-ANSI C the former means function arguments are unspecified, not that there are no arguments. This is related to the current issue in a way that in bug report is used int ext(); which causes adding the bitcast. If you replace it with int ext(void); - everything works fine as the static function is renamed.

This is incomplete explanation that glosses over many details but it should give you the overall idea. For more information you can also check warning -Wstrict-prototypes.

[efriedma]

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

The crucial part is distinction between int ext(); and int ext(void);.

Yes, this is exactly right.

[tejohnson]

In D39356#917074, @efriedma wrote:

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

The crucial part is distinction between int ext(); and int ext(void);.

Yes, this is exactly right.

Got it, thanks.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

283 ↗

(On Diff #121771)

I'm a little worried that we may have a case with CalledValue==null (presumably it wasn't an accident that we were checking for a null CalledValue before). Anyone know if that is valid?

[dexonsmith]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

283 ↗

(On Diff #121771)

In the absence of a failing testcase, one way to find such a case is to commit the assertion and wait for it to fire.

[vsapsai]

In D39356#917108, @tejohnson wrote:

In D39356#917074, @efriedma wrote:

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

; Function Attrs: minsize optsize
declare i32 @f(...) #1

; Function Attrs: minsize optsize
declare i32 @ext(...) #1

But I haven't found references to be used for should-promote decision, so static function wasn't promoted and wasn't renamed.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

283 ↗

(On Diff #121771)

It was suspicious to me that assertion wasn't triggered. I rebased on top of master and made clean build - still no assertion.

[tejohnson]

In D39356#918395, @vsapsai wrote:

In D39356#917108, @tejohnson wrote:

In D39356#917074, @efriedma wrote:

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

Presumably @f is added to the references of @ext's FunctionSummary?

; Function Attrs: minsize optsize
declare i32 @f(...) #1

; Function Attrs: minsize optsize
declare i32 @ext(...) #1

But I haven't found references to be used for should-promote decision, so static function wasn't promoted and wasn't renamed.

They are used here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#328
Then when ext() is imported, its references (which should include @f if we are in fact adding the reference edge for it) would be added to the ExportList. That ExportList is what controls promotion decisions.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

283 ↗

(On Diff #121771)

It's possible that I added it in unfounded paranoia. Let's leave as you have and revisit if it shows up in an actual assertion

[vsapsai]

In D39356#918445, @tejohnson wrote:

In D39356#918395, @vsapsai wrote:

In D39356#917108, @tejohnson wrote:

In D39356#917074, @efriedma wrote:

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

Presumably @f is added to the references of @ext's FunctionSummary?

Sorry, I've misunderstood you. @ext seems to be totally fine. It has @f as a call edge and not as a reference (because of isCallee() check). The problem is in @main. Without my change it has @ext only as a reference, but not as call edge. With my change it has it both as reference and as call edge.

; Function Attrs: minsize optsize
declare i32 @f(...) #1

; Function Attrs: minsize optsize
declare i32 @ext(...) #1

But I haven't found references to be used for should-promote decision, so static function wasn't promoted and wasn't renamed.

They are used here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#328
Then when ext() is imported, its references (which should include @f if we are in fact adding the reference edge for it) would be added to the ExportList. That ExportList is what controls promotion decisions.

I see. When we were importing @ext, we bailed out early, at DefinedGVSummaries.count(VI.getGUID()) http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#254
And when we were importing @main, without my change we did nothing in computeImportForFunction as Summary.calls() was empty.

[tejohnson]

In D39356#918818, @vsapsai wrote:

In D39356#918445, @tejohnson wrote:

In D39356#918395, @vsapsai wrote:

In D39356#917108, @tejohnson wrote:

In D39356#917074, @efriedma wrote:

The issue here is that we detected that this GV was a Callee, so we didn't add a reference edge.

Are you sure that's what's happening? The isCallee test is an exact equality comparison, so it should fail if we're calling a bitcasted global.

Hmm, I think that is a good question, looking at the code again. @vsapsai, can you see why the reference wasn't recorded in findRefEdges?

It was. At first CurUser was a call with bitcast operand - didn't add to RefEdges because bitcast isn't a GlobalValue but add to Worklist. Then we had bitcast as User and function as operand - we added function to RefEdges because in (!(CS && CS.isCallee(&OI))) ImmutableCallSite CS was evaluated to false.

In the end we created FunctionSummary with Refs

Presumably @f is added to the references of @ext's FunctionSummary?

Sorry, I've misunderstood you. @ext seems to be totally fine. It has @f as a call edge and not as a reference (because of isCallee() check). The problem is in @main. Without my change it has @ext only as a reference, but not as call edge. With my change it has it both as reference and as call edge.

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

; Function Attrs: minsize optsize
declare i32 @f(...) #1

; Function Attrs: minsize optsize
declare i32 @ext(...) #1

But I haven't found references to be used for should-promote decision, so static function wasn't promoted and wasn't renamed.

They are used here: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#328
Then when ext() is imported, its references (which should include @f if we are in fact adding the reference edge for it) would be added to the ExportList. That ExportList is what controls promotion decisions.

I see. When we were importing @ext, we bailed out early, at DefinedGVSummaries.count(VI.getGUID()) http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#254

When trying to import @f into @ext I assume? That makes sense since it is defined in the same module.

And when we were importing @main, without my change we did nothing in computeImportForFunction as Summary.calls() was empty.

Ok, importing into @main? That makes sense without this change.

So overall, LGTM at this point.

[mehdi_amini]

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

I'm confused now: was there or not a reference edges for the internal function before this patch? If yes it seems strange that the issue would be in ld64 because the imported function would reference the renamed function, how could it match the external original f()?

[tejohnson]

In D39356#918908, @mehdi_amini wrote:

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

I'm confused now: was there or not a reference edges for the internal function before this patch? If yes it seems strange that the issue would be in ld64 because the imported function would reference the renamed function, how could it match the external original f()?

Here's my current understanding:

• Due to the bitcast on the call to ext() from main(), there was no call edge and therefore no importing of ext() into main(). main() did however have a reference edge to ext(), and ext() has a call edge to the static f() in that module, so we didn't do incorrect dead stripping, etc.
• Because ext() wasn't imported (due to lack of call edge), the static f() was not promoted/renamed
• There was an issue in ld64 that confused the two f() functions, per @vsapsai:

"OK, I believe I've found a place in ld64 where we adjust fixups and associate them with other atoms based on name only, without checking linkage visibility. It is in AtomSyncer as Steven has helpfully mentioned. That issue is investigated separately."

[vsapsai]

In D39356#918923, @tejohnson wrote:

In D39356#918908, @mehdi_amini wrote:

Oh ok, this is different than what thought the issue was. There isn't a correctness problem with this - the reference edge ensures correctness. It sounds like the only correctness issue then was due to the ld64 issue. (I thought there was also an issue with getting @f promoted when @ext was imported.) There isn't really a correctness issue with having both a reference and call edge though. Ideally we would have just the call edge and no reference edge, but that goes back to the earlier suggestion you had tried (changing the call site to look through pointer casts which had other issues). So I think this is fine. Sorry for the confusion.

I'm confused now: was there or not a reference edges for the internal function before this patch? If yes it seems strange that the issue would be in ld64 because the imported function would reference the renamed function, how could it match the external original f()?

Here's my current understanding:

• Due to the bitcast on the call to ext() from main(), there was no call edge and therefore no importing of ext() into main(). main() did however have a reference edge to ext(), and ext() has a call edge to the static f() in that module, so we didn't do incorrect dead stripping, etc.
• Because ext() wasn't imported (due to lack of call edge), the static f() was not promoted/renamed
• There was an issue in ld64 that confused the two f() functions, per @vsapsai:

"OK, I believe I've found a place in ld64 where we adjust fixups and associate them with other atoms based on name only, without checking linkage visibility. It is in AtomSyncer as Steven has helpfully mentioned. That issue is investigated separately."

That's correct summary.

[efriedma]

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

284

The following testcase triggers this assertion:

define void @caller() {
    tail call void select (i1 icmp eq (i32* @a, i32* null), void ()* @f, void ()* @g)()
    ret void
}
declare void @f()
declare void @g()
@a = extern_weak global i32

[vsapsai]

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

284

Good catch, Eli. In this case CalledValue is not NULL but it is SelectConstantExpr as far as I can tell.

@tejohnson, do you think it's better to return previous skipping code or to handle this case in some special way?

@efriedma, is it based on existing test case or just on your LLVM knowledge? Asking to decide where to put this test. If it is a variation of another test, I would prefer to keep them together.

[efriedma]

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

284

There are a few other regression tests with this IR construct, but not in any directory related to ThinLTO.

[vsapsai]

Posted the fix in https://reviews.llvm.org/D40056

llvm/trunk/lib/Analysis/ModuleSummaryAnalysis.cpp

for (const Instruction &I : BB) {
       // a local value from inline assembly to ensure we don't export a
       // reference (which would require renaming and promotion of the
       // referenced value).
       if (HasLocalsInUsedOrAsm && CI && CI->isInlineAsm())
         HasInlineAsmMaybeReferencingInternal = true;
 
       auto *CalledValue = CS.getCalledValue();
       auto *CalledFunction = CS.getCalledFunction();
+      if (CalledValue && !CalledFunction) {
+        CalledValue = CalledValue->stripPointerCastsNoFollowAliases();
+        // Stripping pointer casts can reveal a called function.
+        CalledFunction = dyn_cast<Function>(CalledValue);
+      }
       // Check if this is an alias to a function. If so, get the
       // called aliasee for the checks below.
       if (auto *GA = dyn_cast<GlobalAlias>(CalledValue)) {
         assert(!CalledFunction && "Expected null called function in callsite for alias");
         CalledFunction = dyn_cast<Function>(GA->getBaseObject());
       }
       // Check if this is a direct call to a known function or a known
       // intrinsic, or an indirect call with profile data.
for (const Instruction &I : BB) {
         // aliasee.
         CallGraphEdges[Index.getOrInsertValueInfo(
                            cast<GlobalValue>(CalledValue))]
             .updateHotness(Hotness);
       } else {
         // Skip inline assembly calls.
         if (CI && CI->isInlineAsm())
           continue;
-        // Skip direct calls.
-        if (!CS.getCalledValue() || isa<Constant>(CS.getCalledValue()))
-          continue;
+        assert(CalledValue && !isa<Constant>(CalledValue) &&
+               "Expected indirect call");
 
         uint32_t NumVals, NumCandidates;
         uint64_t TotalCount;
         auto CandidateProfileData =
             ICallAnalysis.getPromotionCandidatesForInstruction(
                 &I, NumVals, TotalCount, NumCandidates);
         for (auto &Candidate : CandidateProfileData)
           CallGraphEdges[Index.getOrInsertValueInfo(Candidate.Value)]

llvm/trunk/test/Bitcode/thinlto-function-summary-callgraph-cast.ll

+; Test to check the callgraph for calls to casts.
+; RUN: opt -module-summary %s -o %t.o
+; RUN: llvm-bcanalyzer -dump %t.o | FileCheck %s
+; PR34966
+
+; CHECK:       <GLOBALVAL_SUMMARY_BLOCK
+; CHECK-NEXT:    <VERSION
+; "op7=1" is a call to "callee" function.
+; CHECK-NEXT:    <PERMODULE {{.*}} op7=1 op8=[[ALIASID:[0-9]+]]/>
+; CHECK-NEXT:    <PERMODULE {{.*}} op0=[[ALIASEEID:[0-9]+]]
+; CHECK-NEXT:    <ALIAS {{.*}} op0=[[ALIASID]] {{.*}} op2=[[ALIASEEID]]/>
+; CHECK-NEXT:  </GLOBALVAL_SUMMARY_BLOCK>
+
+; ModuleID = 'thinlto-function-summary-callgraph-cast.ll'
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @caller() {
+    call void bitcast (void (...)* @callee to void ()*)()
+    call void bitcast (void (...)* @analias to void ()*)()
+    ret void
+}
+
+declare void @callee(...)
+
+@analias = alias void (...), bitcast (void ()* @aliasee to void (...)*)
+
+define void @aliasee() {
+entry:
+    ret void
+}