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

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 2 13:25:16 PDT 2017


tejohnson added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:247
+      if (CalledValue) {
+        CalledValue = CalledValue->stripPointerCastsNoFollowAliases();
+        // Stripping pointer casts can reveal a called function.
----------------
vsapsai wrote:
> tejohnson wrote:
> > Do we need to do this if CalledFunction is non-NULL? I would think the above condition could be "if (CalledValue and !CalledFunction)".
> It works both with `if (CalledValue)` and with `if (CalledValue and !CalledFunction)`. My goal is to strip pointer casts in all cases, unless the value is NULL. And the condition captures this intention. `if (CalledValue and !CalledFunction)` is more an implementation detail because we know CalledFunction to be of type `Function*` and casts are never of type `Function*`, so `stripPointerCastsNoFollowAliases` and non-NULL CalledFunction are mutually exclusive.
> 
> That was my reasoning, don't have strong opinion about it.
Personally I prefer changing to "if (CalledValue and !CalledFunction)". I think it is clearer because there is no chance of having a cast that needs stripping if we have CalledFunction!=null. If we have a CalledFunction, then we will add the appropriate edges.


================
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:
> > 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.
> 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.

I'm not sure why you say this patch is orthogonal to the miscompile. We need these edges in the summary not only for importing, but also for things like liveness analysis and knowing what to promote as a result of other importing decisions. The fact that no edge got added here meant that we didn't realize the callee function was exported when it's caller was imported. As Mehdi points out, we could also add the callee to the reference edges for the caller, but by adding a call edge we are able to import it and if some other optimization can remove the bitcast or otherwise convert to a "direct" call we would be able to inline it.




https://reviews.llvm.org/D39356





More information about the llvm-commits mailing list