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

[Volodymyr Sapsai via Phabricator via llvm-commits]

vsapsai added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:247
+      if (CalledValue) {
+        CalledValue = CalledValue->stripPointerCastsNoFollowAliases();
+        // Stripping pointer casts can reveal a called function.
----------------
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.


================
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
----------------
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?


https://reviews.llvm.org/D39356