[PATCH] D104599: [WIP][Attributor] Derive AAFunctionReachability attribute.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8248
+    // This is a hack for us to be able to cache queries.
+    auto *This = const_cast<AAFunctionReachabilityFunction *>(this);
+
----------------
Move it down to the use. Also rename it to make clear what this is, e.g. NonConstThis.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8253
+    if (CanReachUnknownCallee)
+      return true;
+
----------------
Eventually we don't want this. We want to check, for example, if an unknown callee can reach `Fn` at all.
We also want to encode domain knowledge about (some) GPU kernels as we know they can only be called "from the outside".
That is, you cannot reach one kernel from another one.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8272
+      if (UnreachableQueries.empty())
+        recieveUpdates(A, Edges);
+
----------------
I don't get why we do this conditionally. If we have an unreachable query already, and then a new one comes in, we still now depend on the state of all AAs we asked in the meantime.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8307
+        Current++;
+      }
+    }
----------------
Swap the cases, and use continue to make it easier to read.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8344
+    }
+    return false;
+  }
----------------
Couldn't we add dependences here, maybe keep track of all the AAs above and then add dependences in the false case. removes complexity I think



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104599