[PATCH] D101578: [Attributor][WIP] Add AAExecutionDomainInfo interface

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2281
+
+  const std::string getAsStr() const override { return "[AAExecutionDomain]"; }
+
----------------
return how many blocks are single threaded of how many, as string: "3/7 BBs thread 0 only"



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2312
+  /// Map between basic blocks and their execution domain.
+  DenseMap<const BasicBlock *, bool> SingleThreadedBBs;
+};
----------------
You could use a set, during initialize put all blocks in the set, remove one if you cannot argue it's single threaded. you know something changed if the size at the end of update is not the size at the beginning. That should make things a lot simpler, whenever you fail to show single threaded, remove it from the set right away, no need to communicate booleans all over the place and check the set all the time to update "changed".


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2333
+    if (!Cmp || !Cmp->isTrueWhenEqual())
+      return false;
+
----------------
also check if it is an equality, this would allow `tid >= 0`, I think.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2369
+    return IsSingleThreaded;
+  };
+
----------------
Move down to the usage, also `IsSingleThreadOnly`


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2380-2382
+  if (A.checkForAllCallSites(PredForCallSite, *this, true, AllCallSitesKnown)) {
+    if (AllCallSitesKnown) {
+      if (!SingleThreadedBBs[&F->getEntryBlock()])
----------------



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2439
+  case IRPosition::IRP_CALL_SITE_RETURNED:
+  case IRPosition::IRP_CALL_SITE:
+  case IRPosition::IRP_FUNCTION:
----------------
unreachable for all but function. See the other createForPosition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101578