[PATCH] D71617: [WIP][NFC][Attributor] noalias attribute deduction fixme

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2518
+      // UsePred checks: getCtxI() i.e. callSite, is reachable from V's user.
+      if (!A.checkForAllUses(UsePred, *this, V, getCtxI())) {
+        LLVM_DEBUG(
----------------
pgode wrote:
> jdoerfert wrote:
> > I don't think you should restrict it to uses reachable from `getCtxI` here. All uses are interesting and reachability is checked from the uses.
> Please correct me if I am wrong, I understand that we don't need, where we were checking whether uses of V are reachable from getCtxI(). 
> if (!A.checkForAllUses(UsePred, *this, V, getCtxI()))
> 
> Instead, we need the regular call which checks for all uses of V. 
> if (!A.checkForAllUses(UsePred, *this, V)
> 
> And in that case, we won't need the ReachableFromI parameter to 'checkForAllUses'. And thus we can have the reachability check only in Pred instead of having one more check in checkForAllUses. 
> Only problem with this would be w.r.t. setting AnyUnreachable, which records dependencies. 
> 
> Instead, we need the regular call which checks for all uses of V.
> if (!A.checkForAllUses(UsePred, *this, V)

Yes.

> And in that case, we won't need the ReachableFromI parameter to 'checkForAllUses'. And thus we can have the reachability check only in Pred instead of having one more check in checkForAllUses. 

Yes.


> Only problem with this would be w.r.t. setting AnyUnreachable, which records dependencies.

You can split the changes to `checkForAllUses` into a separate patch. We want the changes but it is not needed for the `noalias` stuff.


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

https://reviews.llvm.org/D71617