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

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 26 09:29:08 PST 2019


jdoerfert added a comment.

I did write a confusing comment in the last review, below I tried to clarify.



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




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


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

https://reviews.llvm.org/D71617





More information about the llvm-commits mailing list