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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Looks like a reasonable start, some comments:

Hook it up to the globalization to make it easier to test it. We also need negative tests and such, swapped conditions, swapped edges, ...



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3851
+  /// Returns true if HeapToStack conversion is known to be possible.
+  bool isKnownSingleThreaded() const { return getKnown(); }
+
----------------
Comments are copy&paste but I don't assume we need these functions at all.
The interface we want is below/


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3866
+
+  virtual bool isSingleThreadExeuction(const BasicBlock *) = 0;
+
----------------
Nit: take references


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4347
 
+// interested in AA for specific positions.
 ChangeStatus AANoCaptureImpl::updateImpl(Attributor &A) {
----------------
Not needed


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8150
+  Function *F = getAnchorScope();
+  llvm::ReversePostOrderTraversal<Function *> RPOT(F);
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8177
+    return false;
+  };
+
----------------
-Master
+MainThreadOnly or FirstThreadOnly

---

I don't get the TrueBB part, you don't know which successor the block you are looking at is from the code below. Make it "SuccBB" and check the condition first, then check if this is the successor under which the condition is `tid == 0`.

---

Given that we have all the openmp specific functions in here we should move this into OpenMPOpt.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8185
+    bool IsSingleThreaded = true;
+    for (auto I = pred_begin(BB), E = pred_end(BB); I != E; ++I) {
+      if (!IsMasterEdge(dyn_cast<BranchInst>((*I)->getTerminator()), BB))
----------------



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:1141
+    }
+  }
+
----------------
I assume this is only for testing, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101578