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

[Volodymyr Sapsai via Phabricator via llvm-commits]

vsapsai added a comment.

In https://reviews.llvm.org/D39356#918445, @tejohnson wrote:

> In https://reviews.llvm.org/D39356#918395, @vsapsai wrote:
>
> > In https://reviews.llvm.org/D39356#917108, @tejohnson wrote:
> >
> > > In https://reviews.llvm.org/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.


https://reviews.llvm.org/D39356