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

[pankaj gode via Phabricator via llvm-commits]

pgode added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2328
+      }
     }
 
----------------
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. 



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5071
+      }
+    }
+
----------------
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. 


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

https://reviews.llvm.org/D71617