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

[Jay Foad via Phabricator via llvm-commits]

foad 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:
> madhur13490 wrote:
> > 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.
> > 
> > 
> > 
> >  
> > If there is some functionality missing in this patch, then that's a different story.
> 
> The code you added to check callers-via-bitcasts does not respect the Ignore* flags, which could cause problems for existing users of this function. The worklist thing was just a suggestion for how to fix that (and also handle bitcasts-of-bitcasts-... in a natural way, though I'm not sure if they ever occur in practice).
Sorry, I was confused. I see now that you're just adding yet more cases which don't count as "taking the address" of the function, which shouldn't cause any problems. I still think this function is getting too complicated and could do with refactoring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98884