[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#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

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



================
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");
----------------
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.


https://reviews.llvm.org/D39356