[PATCH] D98884: [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/IR/Function.cpp:1630
+          if (const CallBase *CB = dyn_cast<CallBase>(U.getUser()))
+            return CB->isCallee(&U);
+          return false;
----------------
foad wrote:
> rampitec wrote:
> > foad wrote:
> > > This does not respect IgnoreCallbackUses, IgnoreAssumeLikeCalls, IgnoreLLVMUsed. Also it does not handle bitcasts-of-bitcasts.
> > > 
> > > Wouldn't the whole function be better implemented as a worklist algorithm now?
> > > ```
> > > SmallVector<Use &> WorkList(use_begin(), use_end());
> > > while (!WorkList.empty()) {
> > >   U = WorkList.pop_back_val();
> > >   if (isa<BitCast>(U))
> > >     WorkList.push_back(U.use_begin(), U.use_end());
> > >   else if (isa<CallBase>(U))
> > >     ... all the existing logic that handles Ignore* flags ...
> > > }
> > > ```
> > In general yes, but this patch is growing. This would be a good followup change.
> > The pass calls hasAddressTaken() without arguments so that is not exerciseable  here as is. For the same reason I withdraw my request to split the patch. Also note this may change if this is landed: D96179.
> I disagree. You're changing a core API Function::hasAddressTaken in a way that could change the behaviour for all existing callers, whether they like it or not. So I think this part probably should be a separate patch and the implementation should be as rigorous as possible.
> I disagree. You're changing a core API Function::hasAddressTaken in a way that could change the behaviour for all existing callers, whether they like it or not. 

In general sense I do agree agree about the impact and that is why the first diff of this patch had an option to have the new code under an optional flag. Please have a look at the diff and the followup discussion on it.

>So I think this part probably should be a separate patch and the implementation should be as rigorous as possible.

I don't understand this part. Worklist based algorithm is more readble and maintenable but that is another way of implementing what this patch does and is not going to do anything different than this does functionality wise. If there is some functionality missing in this patch, then that's a different story. But I don't see that being a reason to block further progress on this patch.



 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98884/new/

https://reviews.llvm.org/D98884