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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D39356#918818, @vsapsai wrote:

> 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.


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.


https://reviews.llvm.org/D39356