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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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?

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



================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:283
           continue;
-        // Skip direct calls.
-        if (!CS.getCalledValue() || isa<Constant>(CS.getCalledValue()))
-          continue;
+        assert(CalledValue && !isa<Constant>(CalledValue) &&
+               "Expected indirect call");
----------------
vsapsai wrote:
> dexonsmith wrote:
> > tejohnson wrote:
> > > 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?
> > 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.
> It was suspicious to me that assertion wasn't triggered. I rebased on top of master and made clean build - still no assertion.
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


https://reviews.llvm.org/D39356