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

[Eli Friedman via Phabricator via llvm-commits]

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


https://reviews.llvm.org/D39356