[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:2328
+      }
     }
 
----------------
pgode wrote:
> jdoerfert wrote:
> > pgode wrote:
> > > jdoerfert wrote:
> > > > Minor:
> > > > You can asssume V has pointer type so you don't need to check it.
> > > > To get the `CallSiteI` directly you can call `getCtxI()`.
> > > > I think you should even pass `CallSiteI->getNextNode()` to `checkForAllUses` because we don't want to see the uses in the call site.
> > > Thanks for the review comments.  I didn't understand the part:
> > > >> I think you should even pass CallSiteI->getNextNode() to checkForAllUses because we don't want to see the uses in the call site.
> > > 
> > > I thought that, if we don't have any uses of instruction/value passed to CallSite, before the callsite instruction then we can propagate.  CallSiteI->getNextNode() gives me next node after the callsite instruction. Not sure about the reason why we should also have additional call to checkForAllUses.
> > > 
> > > Should we have something like below.
> > > ```
> > > if (!A.checkForAllUses(UsePred, *this, V, getCtxI())  && !A.checkForAllUses(UsePred, *this, V, getCtxI()->getNextNode()) {
> > > .. 
> > > }
> > > ```
> > > 
> > First, I got confused myself in the last comment. I'll explain what my thinking was and then why it was not want we want anyway:
> > I thought:
> >   We have a call `call @foo(%v)` and we want to know if `%v` is used *after* the call.
> >   To do that we would look at used reachable from the instruction after the call because we do not want to see the use of `%v` in the call.
> >   That is however not what we actually want to do.
> > 
> > What we want to do is:
> >   Check if there is any use
> >      - which is not marked `nocapture`, 
> >      - which is not the call instruction, and
> >      - from which we can reach the call instruction.
> >   If such a use exists, the value might have been captured already.
> >   If not, it might only be captured by the call instruction or later.
> >   In the former case we need to give up, in the latter we don't.
> > 
> > To achieve this we need to perform the reachability query in the callback (`UsePred` above). If the User (= `U.getUser()`) is an instruction that might cause the value to escape, it is not the call instruction (= `getCtxI()`), and from the user the call instruction is reachable, we give up. Otherwise we continue scanning.
> > 
> > 
> Thanks a lot for clarifying. That make perfect sense to me. 
> I am working on it and will soon submit the next version. 
> 
Sounds good! Looking forward to it.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5071
+      }
+    }
+
----------------
pgode wrote:
> jdoerfert wrote:
> > You need to check both `ReachableFromI` and `ReachabilityAA` for `null` in the outer conditional.
> > If it is *not* reachable we should skip the user, thus `continue`, otherwise we fall through and call the predicate below.
> > 
> > We should also split the change to `checkForAllUses` from the rest and make it a separate review.
> I agree for the null checks and continue. I missed them both.
> Can I keep this and create separate review after this ? 
> As of now, ReachableFromI not equal nullptr value is passed for noalias deduction only. 
You can split this in two reviews. One for the `checkForAllUses` code and one for the `AANoAlias` code.
You will not even need to set `ReachableFromI` from the `NoAlias` query but we still should keep the functionality to only visit reachable users. It will be useful soon.


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

https://reviews.llvm.org/D71617