[PATCH] D63067: [Attributor] NoAlias on return values.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

More comments. Can you diff it against the master branch so one can easily see all changes?



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:738
+      ImmutableCallSite ICS(RI);
+      auto *NoAliasReturnedAA = A.getAAFor<AANoAliasReturned>(*this, *RI);
 
----------------
jdoerfert wrote:
> The call site should be checked separately, e.g. before you ask for the `AANoAlias` attribute for `RV`. If `RV` is a call site and is `returnDoesNotAlias` all is good. Otherwise, we look at the `AANoAlias`. That should actually suffice (no `Returned` needed) and making it more general might make the code potentially more applicable in the future.
This was not correct. You need to check for a call here explicitly but you want to check `RV`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:903
+    if (ReturnType->isPointerTy())
+      registerAA(*new AANoAliasReturned(F, InfoCache));
+  }
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > Add the `Whitelist` check please.
> I added the check here, since all noalias attrs will have same id, and both returned and noalias returned are only one per fucntion.
> 
The `Whitelist` check for `AANoAliasReturned` should be separate and look for `AANoAliasReturned::ID`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:728
 
-    if (PointerMayBeCaptured(RV, false, false)) {
+    if (PointerMayBeCaptured(RV, false, /* StoreCaptures */ true)) {
       indicatePessimisticFixpoint();
----------------
Please add `/* ReturnCaptures */` and a comment (TODO or FIXME) that mentions we could improve the capture check in two ways. 1) Use the AANoCapture facilities, 2) use the location of the associated return insts.

(I just saw, part of the FIXME is down there already!)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:733
 
-    for (auto RI : ReturnInsts) {
-      ImmutableCallSite ICS(RI);
-      auto *NoAliasReturnedAA = A.getAAFor<AANoAliasReturned>(*this, *RI);
+    ImmutableCallSite ICS(RV);
+    if(ICS && ICS.returnDoesNotAlias())
----------------
I think you should move a check before the capture check that makes sure we actually have a call site here (for now, thus add a FIXME to support more). If we do not have a call site, we can for now not derive `noalias` for the return value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63067