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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:251
+          CalledFunction = dyn_cast<Function>(CalledValue);
+      }
       // Check if this is an alias to a function. If so, get the
----------------
efriedma wrote:
> vsapsai wrote:
> > tejohnson wrote:
> > > mehdi_amini wrote:
> > > > vsapsai wrote:
> > > > > mehdi_amini wrote:
> > > > > > Isn't it enough to just replace ` auto *CalledValue = CS.getCalledValue();` with ` auto *CalledValue = CS.getCalledValue()->stripPointerCastsNoFollowAliases();` ?
> > > > > 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.
> > > > 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.
> > > Agree, this would be a good thing to try.
> > 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?
> 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.
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.


https://reviews.llvm.org/D39356