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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Why should the return values in nonnull.ll be `noalias`?

Please also add more explicit `noalias` return value tests.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:707
+    if (F.returnDoesNotAlias()) {
+      indicatePessimisticFixpoint();
+      return;
----------------
Why a pessimistic fixpoint? It should be optimistic, right?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:723
 
-    if (isa<Argument>(RetVal))
-      return false;
+  for (auto It = AARetValImpl->begin(); It != AARetValImpl->end(); ++It) {
+    Value *RV = It->first;
----------------
In short:
`for (auto &It : *AARetValImpl)`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:725
+    Value *RV = It->first;
+    const SmallPtrSet<ReturnInst *, 2> ReturnInsts = It->second;
 
----------------
Make it a const reference, thus add `&`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:731
 
-ChangeStatus AANoAliasReturned::updateImpl(Attributor &A) {
-  Function &F = getAnchorScope();
+    if (PointerMayBeCaptured(RV, false, false)) {
+      indicatePessimisticFixpoint();
----------------
Stores capture, so make the last one a `true`, also use the `/* argumentname */` syntax to explain the booleans.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:737
+    for (auto RI : ReturnInsts) {
+      ImmutableCallSite ICS(RI);
+      auto *NoAliasReturnedAA = A.getAAFor<AANoAliasReturned>(*this, *RI);
----------------
You want to check `RV` here not the return instructions it can be returned through. The latter is only interesting to improve the escape query above but that is future work.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:738
+      ImmutableCallSite ICS(RI);
+      auto *NoAliasReturnedAA = A.getAAFor<AANoAliasReturned>(*this, *RI);
 
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:903
+    if (ReturnType->isPointerTy())
+      registerAA(*new AANoAliasReturned(F, InfoCache));
+  }
----------------
Add the `Whitelist` check please.


================
Comment at: llvm/test/Transforms/FunctionAttrs/noalias.ll:1
+; RUN: opt -S -attributor < %s | FileCheck %s
+
----------------
Can you rename the file to noalias_returned.ll or returned_noalias.ll and also explicitly enable the attributor via `-attributor-disable=false`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63067