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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 1 15:42:44 PDT 2017


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:
> efriedma wrote:
> > mehdi_amini wrote:
> > > 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.
> > No, the inliner has no special handling; it treats calls where the callee is a bitcast as indirect calls.
> So how do you recommend to address this problem? I've tried to leave CalledFunction NULL and handle it in the branch for indirect calls. But IndirectCallPromotionAnalysis doesn't return any promotion candidates because call instruction has no MD_prof metadata. As the result, nothing is added to the call graph.
Well, in terms of fixing the PR34966 miscompile, this patch is essentially orthogonal.  ThinLTO importing should generate correct code whether or not a function pointer is called directly.

In terms of trying to improve performance for pre-ANSI C code, I guess this patch is fine?  It looks like adding the edge won't do any harm even if the rest of the optimizer continues to treat it as an indirect call.


https://reviews.llvm.org/D39356





More information about the llvm-commits mailing list