[PATCH] D141164: [AAUnderlyingObjects] Introduce an AA for getting underlying objects of a pointer

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

The failing test are probably because of the scope (Intra vs Inter). Other comments inlined as well.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:344
+  const auto &AA = A.getAAFor<AAUnderlyingObjects>(
+      QueryingAA, IRPosition::value(Ptr), DepClassTy::REQUIRED);
+  if (!AA.getState().isValidState()) {
----------------



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:348
         dbgs() << "Underlying objects stored into could not be determined\n";);
     return false;
   }
----------------
We should make it easier for users and check the state in the foreach function. Move the debug output to the call below though.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:361-362
+
+  auto Pred = [&](Value &V) {
+    Value *Obj = &V;
     LLVM_DEBUG(dbgs() << "Visit underlying object " << *Obj << "\n");
----------------
Adjust the rest accordingly. No need for two variables pointing to one thing.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8219
+  const auto &AA = A.getAAFor<AAUnderlyingObjects>(
+      *this, IRPosition::value(Ptr), DepClassTy::REQUIRED);
+  if (!AA.forallUnderlyingObjects(Pred)) {
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11234
+    if (!A.getAssumedSimplifiedValues(IRPosition::value(Ptr), *this, Values,
+                                      AA::ValueScope::Intraprocedural,
+                                      UsedAssumedInformation))
----------------
I think we should let the user pass this and update for the values the user asked, so potentially twice (intra and inter). We can add smarts to avoid one of them sometimes but I wouldn't for now.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11249
+        auto Pred = [&](Value &V) {
+          Objects.insert(&V);
+          return true;
----------------
We should put it in Values, this can be recursive.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11254
+        if (!OtherAA.forallUnderlyingObjects(Pred))
+          return indicatePessimisticFixpoint();
+
----------------
No need to give up. Just use `UO`. Since this can only happen if the AA couldn't determine all underlying objects, we could probably also add an assertion instead. If the AA is not able to provide information it should just return the queried value.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11277
+    if (!isValidState())
+      return false;
+
----------------
We can just call Pred on the associatedValue. It's always correct and conservative.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11291
+    const auto &AA = A.getAAFor<AAUnderlyingObjects>(
+        *this, IRPosition::value(V), DepClassTy::REQUIRED);
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:11300
+    if (!AA.forallUnderlyingObjects(Pred))
+      indicatePessimisticFixpoint();
+
----------------
With the proposed changes above, this can be an assertion as it should always work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141164