[PATCH] D84665: [Attributor] Implement AAReachability

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for working on this! Reachability should be used more and with an actual optimistic implementation it makes sense. The next user (in addition to AANoAlias) could be value simplification, for load/store simplification. Similarly to the noalias case we can preserve other properties for a program point if it is not reachable from all potential violations (I am thinking a use of a `dereferenceable` argument that cannot be reached from a function call that is not `nofree`. This makes sense once we split `dereferenceable_globally` from `dereferenceable`.) Let's get this in first though ;)

Below some comments.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3262
+  /// See AbstractAttribute::initialize(...).
+  void initialize(Attributor &A) override { indicatePessimisticFixpoint(); }
+
----------------
This seems odd. If there is no reason for this version let's remove both initialize methods.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3267
+    return indicatePessimisticFixpoint();
+  }
+};
----------------
Also not needed as far as I can tell.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3281-3284
+    if (SetInsertResult == true) {
+      LLVM_DEBUG(dbgs() << "[AAReachability] Add edge : " << *FromBB << " -> "
+                        << *ToBB << "\n");
+    }
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3333
+        Changed = Changed | addEdge(FromBB, I->getParent());
+      }
+    }
----------------
This looks like the AAIsDead code, right? Maybe we can instead expose some edge based interface, e.g., `AAIsDead::isEdgeDead(BasicBlock, BasicBlock)` or `AAIsDead::getLiveOutEdges(BasicBlock)`. WDYT?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:3375
+  /// Map from a basic block to its reachable successor basic blocks
+  DenseMap<const BasicBlock *, SetVector<const BasicBlock *>> SuccessorGraph;
+};
----------------
We need to profile this but similar maps were causing memory problems before. Make the value a `std::unique_ptr` and that should already help. I'm not sure about this cache in general. I guess it mainly depends on the usage of this AA later on. For now we can keep it but we should invest in some time tracking for AAs at some point and thereby determine if this is too costly. If that is the case we could "cache" only the queries that were asked and recompute them in every update, not the entire graph.


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

https://reviews.llvm.org/D84665