[SandboxVec][DAG] Build actual dependencies #111094

vporpo · 2024-10-04T03:46:40Z

This patch implements actual dependencies checking using BatchAA. This adds memory dep edges between MemDGNodes.

llvmbot · 2024-10-04T03:47:20Z

@llvm/pr-subscribers-llvm-transforms

Author: vporpo (vporpo)

Changes

This patch implements actual dependencies checking using BatchAA. This adds memory dep edges between MemDGNodes.

Patch is 24.88 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/111094.diff

5 Files Affected:

(modified) llvm/include/llvm/SandboxIR/Utils.h (+8)
(modified) llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h (+40-2)
(modified) llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h (+7-5)
(modified) llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp (+123-12)
(modified) llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp (+309-5)

diff --git a/llvm/include/llvm/SandboxIR/Utils.h b/llvm/include/llvm/SandboxIR/Utils.h
index e50621b4c1228e..8842fe5859d18f 100644
--- a/llvm/include/llvm/SandboxIR/Utils.h
+++ b/llvm/include/llvm/SandboxIR/Utils.h
@@ -12,6 +12,7 @@
 #ifndef LLVM_SANDBOXIR_UTILS_H
 #define LLVM_SANDBOXIR_UTILS_H
 
+#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/LoopAccessAnalysis.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/ScalarEvolution.h"
@@ -99,6 +100,13 @@ class Utils {
       return false;
     return *Diff > 0;
   }
+
+  /// Equivalent to BatchAA::getModRefInfo().
+  static ModRefInfo
+  aliasAnalysisGetModRefInfo(BatchAAResults &BatchAA, const Instruction *I,
+                             const std::optional<MemoryLocation> &OptLoc) {
+    return BatchAA.getModRefInfo(cast<llvm::Instruction>(I->Val), OptLoc);
+  }
 };
 
 } // namespace llvm::sandboxir
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h
index 6333f0b81f9c33..6a03cd4efc6563 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h
@@ -24,6 +24,7 @@
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/iterator_range.h"
+#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/SandboxIR/Instruction.h"
 #include "llvm/SandboxIR/IntrinsicInst.h"
 #include "llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h"
@@ -47,6 +48,7 @@ class DGNode {
   // TODO: Use a PointerIntPair for SubclassID and I.
   /// For isa/dyn_cast etc.
   DGNodeID SubclassID;
+  // TODO: Move MemPreds to MemDGNode.
   /// Memory predecessors.
   DenseSet<MemDGNode *> MemPreds;
 
@@ -86,13 +88,20 @@ class DGNode {
            (!(II = dyn_cast<IntrinsicInst>(I)) || isMemIntrinsic(II));
   }
 
+  /// \Returns true if \p I is fence like. It excludes non-mem intrinsics.
+  static bool isFenceLike(Instruction *I) {
+    IntrinsicInst *II;
+    return I->isFenceLike() &&
+           (!(II = dyn_cast<IntrinsicInst>(I)) || isMemIntrinsic(II));
+  }
+
   /// \Returns true if \p I is a memory dependency candidate instruction.
   static bool isMemDepNodeCandidate(Instruction *I) {
     AllocaInst *Alloca;
     return isMemDepCandidate(I) ||
            ((Alloca = dyn_cast<AllocaInst>(I)) &&
             Alloca->isUsedWithInAlloca()) ||
-           isStackSaveOrRestoreIntrinsic(I);
+           isStackSaveOrRestoreIntrinsic(I) || isFenceLike(I);
   }
 
   Instruction *getInstruction() const { return I; }
@@ -159,8 +168,37 @@ class DependencyGraph {
   /// The DAG spans across all instructions in this interval.
   Interval<Instruction> DAGInterval;
 
+  std::unique_ptr<BatchAAResults> BatchAA;
+
+  enum class DependencyType {
+    RAW,   ///> Read After Write
+    WAW,   ///> Write After Write
+    RAR,   ///> Read After Read
+    WAR,   ///> Write After Read
+    CTRL,  ///> Dependencies related to PHIs or Terminators
+    OTHER, ///> Currently used for stack related instrs
+    NONE,  ///> No memory/other dependency
+  };
+  /// \Returns the dependency type depending on whether instructions may
+  /// read/write memory or whether they are some specific opcode-related
+  /// restrictions.
+  /// Note: It does not check whether a memory dependency is actually correct,
+  /// as it won't call AA. Therefore it returns the worst-case dep type.
+  static DependencyType getRoughDepType(Instruction *FromI, Instruction *ToI);
+
+  // TODO: Implement AABudget.
+  /// \Returns true if there is a memory/other dependency \p SrcI->DstI.
+  bool alias(Instruction *SrcI, Instruction *DstI, DependencyType DepType);
+
+  bool hasDep(sandboxir::Instruction *SrcI, sandboxir::Instruction *DstI);
+
+  /// Go through all mem nodes in \p SrcScanRange and try to add dependencies to
+  /// \p DstN.
+  void scanAndAddDeps(DGNode &DstN, const Interval<MemDGNode> &SrcScanRange);
+
 public:
-  DependencyGraph() {}
+  DependencyGraph(AAResults &AA)
+      : BatchAA(std::make_unique<BatchAAResults>(AA)) {}
 
   DGNode *getNode(Instruction *I) const {
     auto It = InstrToNodeMap.find(I);
diff --git a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h
index 8f25ad109f6a61..82aed630a52e2c 100644
--- a/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h
+++ b/llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/Interval.h
@@ -57,7 +57,7 @@ template <typename T, typename IntervalType> class IntervalIterator {
   }
   IntervalIterator &operator--() {
     // `I` is nullptr for end() when To is the BB terminator.
-    I = I != nullptr ? I->getPrevNode() : R.To;
+    I = I != nullptr ? I->getPrevNode() : R.bottom();
     return *this;
   }
   IntervalIterator operator--(int) {
@@ -109,14 +109,16 @@ template <typename T> class Interval {
   T *bottom() const { return To; }
 
   using iterator = IntervalIterator<T, Interval>;
-  using const_iterator = IntervalIterator<const T, const Interval>;
   iterator begin() { return iterator(From, *this); }
   iterator end() {
     return iterator(To != nullptr ? To->getNextNode() : nullptr, *this);
   }
-  const_iterator begin() const { return const_iterator(From, *this); }
-  const_iterator end() const {
-    return const_iterator(To != nullptr ? To->getNextNode() : nullptr, *this);
+  iterator begin() const {
+    return iterator(From, const_cast<Interval &>(*this));
+  }
+  iterator end() const {
+    return iterator(To != nullptr ? To->getNextNode() : nullptr,
+                    const_cast<Interval &>(*this));
   }
   /// Equality.
   bool operator==(const Interval &Other) const {
diff --git a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp
index 10da07c24940dd..1ea17ad03a64cd 100644
--- a/llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp
+++ b/llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp
@@ -8,8 +8,9 @@
 
 #include "llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/SandboxIR/Utils.h"
 
-using namespace llvm::sandboxir;
+namespace llvm::sandboxir {
 
 #ifndef NDEBUG
 void DGNode::print(raw_ostream &OS, bool PrintDeps) const {
@@ -50,19 +51,119 @@ MemDGNodeIntervalBuilder::make(const Interval<Instruction> &Instrs,
                              cast<MemDGNode>(DAG.getNode(MemBotI)));
 }
 
+DependencyGraph::DependencyType
+DependencyGraph::getRoughDepType(Instruction *FromI, Instruction *ToI) {
+  if (FromI->mayWriteToMemory()) {
+    if (ToI->mayReadFromMemory())
+      return DependencyType::RAW;
+    if (ToI->mayWriteToMemory())
+      return DependencyType::WAW;
+  }
+  if (FromI->mayReadFromMemory()) {
+    if (ToI->mayWriteToMemory())
+      return DependencyType::WAR;
+    if (ToI->mayReadFromMemory())
+      return DependencyType::RAR;
+  }
+  if (isa<sandboxir::PHINode>(FromI) || isa<sandboxir::PHINode>(ToI))
+    return DependencyType::CTRL;
+  if (ToI->isTerminator())
+    return DependencyType::CTRL;
+  if (DGNode::isStackSaveOrRestoreIntrinsic(FromI) ||
+      DGNode::isStackSaveOrRestoreIntrinsic(ToI))
+    return DependencyType::OTHER;
+  return DependencyType::NONE;
+}
+
+static bool isOrdered(Instruction *I) {
+  auto IsOrdered = [](Instruction *I) {
+    if (auto *LI = dyn_cast<LoadInst>(I))
+      return !LI->isUnordered();
+    if (auto *SI = dyn_cast<StoreInst>(I))
+      return !SI->isUnordered();
+    if (DGNode::isFenceLike(I))
+      return true;
+    return false;
+  };
+  bool Is = IsOrdered(I);
+  assert((!Is || DGNode::isMemDepCandidate(I)) &&
+         "An ordered instruction must be a MemDepCandidate!");
+  return Is;
+}
+
+bool DependencyGraph::alias(Instruction *SrcI, Instruction *DstI,
+                            DependencyType DepType) {
+  std::optional<MemoryLocation> DstLocOpt =
+      Utils::memoryLocationGetOrNone(DstI);
+  if (!DstLocOpt)
+    return true;
+  // Check aliasing.
+  assert((SrcI->mayReadFromMemory() || SrcI->mayWriteToMemory()) &&
+         "Expected a mem instr");
+  // TODO: Check AABudget
+  ModRefInfo SrcModRef =
+      isOrdered(SrcI)
+          ? ModRefInfo::Mod
+          : Utils::aliasAnalysisGetModRefInfo(*BatchAA, SrcI, *DstLocOpt);
+  switch (DepType) {
+  case DependencyType::RAW:
+  case DependencyType::WAW:
+    return isModSet(SrcModRef);
+  case DependencyType::WAR:
+    return isRefSet(SrcModRef);
+  default:
+    llvm_unreachable("Expected only RAW, WAW and WAR!");
+  }
+}
+
+bool DependencyGraph::hasDep(Instruction *SrcI, Instruction *DstI) {
+  DependencyType RoughDepType = getRoughDepType(SrcI, DstI);
+  switch (RoughDepType) {
+  case DependencyType::RAR:
+    return false;
+  case DependencyType::RAW:
+  case DependencyType::WAW:
+  case DependencyType::WAR:
+    return alias(SrcI, DstI, RoughDepType);
+  case DependencyType::CTRL:
+    // Adding actual dep edges from PHIs/to terminator would just create too
+    // many edges, which would be bad for compile-time.
+    // So we ignore them in the DAG formation but handle them in the
+    // scheduler, while sorting the ready list.
+    return false;
+  case DependencyType::OTHER:
+    return true;
+  case DependencyType::NONE:
+    return false;
+  }
+}
+
+void DependencyGraph::scanAndAddDeps(DGNode &DstN,
+                                     const Interval<MemDGNode> &SrcScanRange) {
+  assert(isa<MemDGNode>(DstN) &&
+         "DstN is the mem dep destination, so it must be mem");
+  Instruction *DstI = DstN.getInstruction();
+  // Walk up the instruction chain from ScanRange bottom to top, looking for
+  // memory instrs that may alias.
+  for (MemDGNode &SrcN : reverse(SrcScanRange)) {
+    Instruction *SrcI = SrcN.getInstruction();
+    if (hasDep(SrcI, DstI))
+      DstN.addMemPred(&SrcN);
+  }
+}
+
 Interval<Instruction> DependencyGraph::extend(ArrayRef<Instruction *> Instrs) {
   if (Instrs.empty())
     return {};
-  // TODO: For now create a chain of dependencies.
-  Interval<Instruction> Interval(Instrs);
-  auto *TopI = Interval.top();
-  auto *BotI = Interval.bottom();
-  DGNode *LastN = getOrCreateNode(TopI);
+
+  Interval<Instruction> InstrInterval(Instrs);
+
+  DGNode *LastN = getOrCreateNode(InstrInterval.top());
+  // Create DGNodes for all instrs in Interval to avoid future Instruction to
+  // DGNode lookups.
   MemDGNode *LastMemN = dyn_cast<MemDGNode>(LastN);
-  for (Instruction *I = TopI->getNextNode(), *E = BotI->getNextNode(); I != E;
-       I = I->getNextNode()) {
-    auto *N = getOrCreateNode(I);
-    N->addMemPred(LastMemN);
+  for (Instruction &I : drop_begin(InstrInterval)) {
+    auto *N = getOrCreateNode(&I);
     // Build the Mem node chain.
     if (auto *MemN = dyn_cast<MemDGNode>(N)) {
       MemN->setPrevNode(LastMemN);
@@ -70,9 +171,17 @@ Interval<Instruction> DependencyGraph::extend(ArrayRef<Instruction *> Instrs) {
         LastMemN->setNextNode(MemN);
       LastMemN = MemN;
     }
-    LastN = N;
   }
-  return Interval;
+  // Create the dependencies.
+  auto DstRange = MemDGNodeIntervalBuilder::make(InstrInterval, *this);
+  for (MemDGNode &DstN : DstRange) {
+    auto DiffRanges = DstRange - Interval<MemDGNode>(&DstN, DstRange.bottom());
+    assert(DiffRanges.size() == 1u && "Expect single difference!");
+    const auto &SrcRange = DiffRanges[0];
+    scanAndAddDeps(DstN, SrcRange);
+  }
+
+  return InstrInterval;
 }
 
 #ifndef NDEBUG
@@ -95,3 +204,5 @@ void DependencyGraph::dump() const {
   dbgs() << "\n";
 }
 #endif // NDEBUG
+
+} // namespace llvm::sandboxir
diff --git a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp
index fb8d3780684f80..e2f16919a5cddd 100644
--- a/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp
@@ -7,7 +7,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h"
+#include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/BasicAliasAnalysis.h"
+#include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/DataLayout.h"
+#include "llvm/IR/Dominators.h"
 #include "llvm/SandboxIR/Context.h"
 #include "llvm/SandboxIR/Function.h"
 #include "llvm/SandboxIR/Instruction.h"
@@ -20,6 +26,10 @@ using namespace llvm;
 struct DependencyGraphTest : public testing::Test {
   LLVMContext C;
   std::unique_ptr<Module> M;
+  std::unique_ptr<AssumptionCache> AC;
+  std::unique_ptr<DominatorTree> DT;
+  std::unique_ptr<BasicAAResult> BAA;
+  std::unique_ptr<AAResults> AA;
 
   void parseIR(LLVMContext &C, const char *IR) {
     SMDiagnostic Err;
@@ -27,6 +37,24 @@ struct DependencyGraphTest : public testing::Test {
     if (!M)
       Err.print("DependencyGraphTest", errs());
   }
+
+  AAResults &getAA(llvm::Function &LLVMF) {
+    TargetLibraryInfoImpl TLII;
+    TargetLibraryInfo TLI(TLII);
+    AA = std::make_unique<AAResults>(TLI);
+    AC = std::make_unique<AssumptionCache>(LLVMF);
+    DT = std::make_unique<DominatorTree>(LLVMF);
+    BAA = std::make_unique<BasicAAResult>(M->getDataLayout(), LLVMF, TLI, *AC,
+                                          DT.get());
+    AA->addAAResult(*BAA);
+    return *AA;
+  }
+  /// \Returns true if there is a dependency: SrcN->DstN.
+  bool dependency(sandboxir::DGNode *SrcN, sandboxir::DGNode *DstN) {
+    const auto &Preds = DstN->memPreds();
+    auto It = find(Preds, SrcN);
+    return It != Preds.end();
+  }
 };
 
 TEST_F(DependencyGraphTest, isStackSaveOrRestoreIntrinsic) {
@@ -151,6 +179,7 @@ define void @foo(i8 %v1, ptr %ptr) {
 )IR");
   llvm::Function *LLVMF = &*M->getFunction("foo");
   sandboxir::Context Ctx(C);
+
   auto *F = Ctx.createFunction(LLVMF);
   auto *BB = &*F->begin();
   auto It = BB->begin();
@@ -165,7 +194,7 @@ define void @foo(i8 %v1, ptr %ptr) {
   auto *Call = cast<sandboxir::CallInst>(&*It++);
   auto *Ret = cast<sandboxir::ReturnInst>(&*It++);
 
-  sandboxir::DependencyGraph DAG;
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
   DAG.extend({&*BB->begin(), BB->getTerminator()});
   EXPECT_TRUE(isa<llvm::sandboxir::MemDGNode>(DAG.getNode(Store)));
   EXPECT_TRUE(isa<llvm::sandboxir::MemDGNode>(DAG.getNode(Load)));
@@ -195,7 +224,7 @@ define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
   auto *S0 = cast<sandboxir::StoreInst>(&*It++);
   auto *S1 = cast<sandboxir::StoreInst>(&*It++);
   auto *Ret = cast<sandboxir::ReturnInst>(&*It++);
-  sandboxir::DependencyGraph DAG;
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
   auto Span = DAG.extend({&*BB->begin(), BB->getTerminator()});
   // Check extend().
   EXPECT_EQ(Span.top(), &*BB->begin());
@@ -214,7 +243,7 @@ define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
   // Check memPreds().
   EXPECT_TRUE(N0->memPreds().empty());
   EXPECT_THAT(N1->memPreds(), testing::ElementsAre(N0));
-  EXPECT_THAT(N2->memPreds(), testing::ElementsAre(N1));
+  EXPECT_TRUE(N2->memPreds().empty());
 }
 
 TEST_F(DependencyGraphTest, MemDGNode_getPrevNode_getNextNode) {
@@ -236,7 +265,7 @@ define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
   auto *S1 = cast<sandboxir::StoreInst>(&*It++);
   [[maybe_unused]] auto *Ret = cast<sandboxir::ReturnInst>(&*It++);
 
-  sandboxir::DependencyGraph DAG;
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
   DAG.extend({&*BB->begin(), BB->getTerminator()});
 
   auto *S0N = cast<sandboxir::MemDGNode>(DAG.getNode(S0));
@@ -270,7 +299,7 @@ define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
   auto *S1 = cast<sandboxir::StoreInst>(&*It++);
   auto *Ret = cast<sandboxir::ReturnInst>(&*It++);
 
-  sandboxir::DependencyGraph DAG;
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
   DAG.extend({&*BB->begin(), BB->getTerminator()});
 
   auto *S0N = cast<sandboxir::MemDGNode>(DAG.getNode(S0));
@@ -313,3 +342,278 @@ define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
       getPtrVec(sandboxir::MemDGNodeIntervalBuilder::make({Add0, Add0}, DAG)),
       testing::ElementsAre());
 }
+
+TEST_F(DependencyGraphTest, AliasingStores) {
+  parseIR(C, R"IR(
+define void @foo(ptr %ptr, i8 %v0, i8 %v1) {
+  store i8 %v0, ptr %ptr
+  store i8 %v1, ptr %ptr
+  ret void
+}
+)IR");
+  llvm::Function *LLVMF = &*M->getFunction("foo");
+  sandboxir::Context Ctx(C);
+  auto *F = Ctx.createFunction(LLVMF);
+  auto *BB = &*F->begin();
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
+  DAG.extend({&*BB->begin(), BB->getTerminator()});
+  auto It = BB->begin();
+  auto *Store0N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *Store1N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *RetN = DAG.getNode(cast<sandboxir::ReturnInst>(&*It++));
+  EXPECT_TRUE(Store0N->memPreds().empty());
+  EXPECT_THAT(Store1N->memPreds(), testing::ElementsAre(Store0N));
+  EXPECT_TRUE(RetN->memPreds().empty());
+}
+
+TEST_F(DependencyGraphTest, NonAliasingStores) {
+  parseIR(C, R"IR(
+define void @foo(ptr noalias %ptr0, ptr noalias %ptr1, i8 %v0, i8 %v1) {
+  store i8 %v0, ptr %ptr0
+  store i8 %v1, ptr %ptr1
+  ret void
+}
+)IR");
+  llvm::Function *LLVMF = &*M->getFunction("foo");
+  sandboxir::Context Ctx(C);
+  auto *F = Ctx.createFunction(LLVMF);
+  auto *BB = &*F->begin();
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
+  DAG.extend({&*BB->begin(), BB->getTerminator()});
+  auto It = BB->begin();
+  auto *Store0N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *Store1N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *RetN = DAG.getNode(cast<sandboxir::ReturnInst>(&*It++));
+  // We expect no dependencies because the stores don't alias.
+  EXPECT_TRUE(Store0N->memPreds().empty());
+  EXPECT_TRUE(Store1N->memPreds().empty());
+  EXPECT_TRUE(RetN->memPreds().empty());
+}
+
+TEST_F(DependencyGraphTest, VolatileLoads) {
+  parseIR(C, R"IR(
+define void @foo(ptr noalias %ptr0, ptr noalias %ptr1) {
+  %ld0 = load volatile i8, ptr %ptr0
+  %ld1 = load volatile i8, ptr %ptr1
+  ret void
+}
+)IR");
+  llvm::Function *LLVMF = &*M->getFunction("foo");
+  sandboxir::Context Ctx(C);
+  auto *F = Ctx.createFunction(LLVMF);
+  auto *BB = &*F->begin();
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
+  DAG.extend({&*BB->begin(), BB->getTerminator()});
+  auto It = BB->begin();
+  auto *Ld0N = DAG.getNode(cast<sandboxir::LoadInst>(&*It++));
+  auto *Ld1N = DAG.getNode(cast<sandboxir::LoadInst>(&*It++));
+  auto *RetN = DAG.getNode(cast<sandboxir::ReturnInst>(&*It++));
+  EXPECT_TRUE(Ld0N->memPreds().empty());
+  EXPECT_THAT(Ld1N->memPreds(), testing::ElementsAre(Ld0N));
+  EXPECT_TRUE(RetN->memPreds().empty());
+}
+
+TEST_F(DependencyGraphTest, VolatileSotres) {
+  parseIR(C, R"IR(
+define void @foo(ptr noalias %ptr0, ptr noalias %ptr1, i8 %v) {
+  store volatile i8 %v, ptr %ptr0
+  store volatile i8 %v, ptr %ptr1
+  ret void
+}
+)IR");
+  llvm::Function *LLVMF = &*M->getFunction("foo");
+  sandboxir::Context Ctx(C);
+  auto *F = Ctx.createFunction(LLVMF);
+  auto *BB = &*F->begin();
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
+  DAG.extend({&*BB->begin(), BB->getTerminator()});
+  auto It = BB->begin();
+  auto *Store0N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *Store1N = DAG.getNode(cast<sandboxir::StoreInst>(&*It++));
+  auto *RetN = DAG.getNode(cast<sandboxir::ReturnInst>(&*It++));
+  EXPECT_TRUE(Store0N->memPreds().empty());
+  EXPECT_THAT(Store1N->memPreds(), testing::ElementsAre(Store0N));
+  EXPECT_TRUE(RetN->memPreds().empty());
+}
+
+TEST_F(DependencyGraphTest, Call) {
+  parseIR(C, R"IR(
+declare void @bar1()
+declare void @bar2()
+define void @foo(float %v1, float %v2) {
+  call void @bar1()
+  %add = fadd float %v1, %v2
+  call void @bar2()
+  ret void
+}
+)IR");
+  Function *LLVMF = M->getFunction("foo");
+
+  sandboxir::Context Ctx(C);
+  auto *F = Ctx.createFunction(LLVMF);
+  auto *BB = &*F->begin();
+
+  sandboxir::DependencyGraph DAG(getAA(*LLVMF));
+  DAG.extend({&*BB->begin(), BB->getTerminator()->getPrevNode()});
+
+  auto It = BB->begin();
+  auto *Call1N = DAG.getNode(&*It++);
+  auto *AddN = DAG.getNode(&*It++);
+  auto *Call2N = DAG.getNode(&*It++);
+
+  EXPECT_THAT(Call1N->memPre...
[truncated]

vporpo · 2024-10-04T16:08:21Z

Rebase

tmsri

Some initial comments.

tmsri · 2024-10-08T05:22:24Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

@@ -86,13 +88,20 @@ class DGNode {
           (!(II = dyn_cast<IntrinsicInst>(I)) || isMemIntrinsic(II));
  }

+  /// \Returns true if \p I is fence like. It excludes non-mem intrinsics.
+  static bool isFenceLike(Instruction *I) {


Why is this not in SandboxIR/Utils.h?

I moved these functions here after introducing sandboxir::IntrinsicInst. These functions previously required accesss to LLVM IR, which was why they were in Utils.

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

tmsri · 2024-10-08T06:04:47Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+      return DependencyType::WAR;
+    if (ToI->mayReadFromMemory())
+      return DependencyType::RAR;
+  }


Why not else-if in the second outer if for FromI?

Why is ToI checked first for Read in the first-if and for write in the second, any implicit ordering? If ToI is not mem read/write this code will still go through all the checks? Can it both read and write memory, if so can we make it simpler, something along these lines where write takes precedence?

enum AccessType { NONE, READ, WRITE, } FromMemType = NONE; // If it could both read and write, write takes precedence if (FromI->mayWriteToMemory()) FromMemType = AccessType::WRITE; else if (FromI->mayReadFromMemory()) FromMemType = AccessType::READ; if (FromMemType != NONE) { // Check Write First if (ToI->mayWriteToMemory()) return FromMemType == AccessType::READ ? WAR : WAW; if (ToI->mayReadFromMemory()) return FromMemType == AccessType::READ ? RAR : RAW; }

Why not else-if in the second outer if for FromI?

else-if should work. DONE.

Why is ToI checked first for Read in the first-if and for write in the second, any implicit ordering?

The order matters in the second outer if because one of them is a RAR (no dependency) and the other one is a WAR which is a dependency. The order in the first outer if shouldn't matter.

If ToI is not mem read/write this code will still go through all the checks?

Currently yes, we could speed it up perhaps by checking mayReadOrWriteMemory() for both and skip some of the code if neither is memory. I will add a TODO for it to do this change after we have enough tests and some benchmarks for the DAG.

Can it both read and write memory,

Yes

if so can we make it simpler, something along these lines where write takes precedence?

Yeah, this should also work, but it seems to be doing the same number of checks while being more verbose (because of the enum declaration). I don't feel too strongly about keeping the original one though.

This is fine, a comment that the order matters in the second case would resolve it.

aeubanks · 2024-10-08T18:19:04Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+  // TODO: Perhaps compile-time improvement by skipping if neither is mem?
+  if (FromI->mayWriteToMemory()) {
+    if (ToI->mayReadFromMemory())
+      return DependencyType::RAW;


is RAW more conservative than WAW?

They are about the same in the sense that you have to force ordering for all RAW, WAW or WAR.

aeubanks · 2024-10-08T18:21:31Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

+    WAW,   ///> Write After Write
+    RAR,   ///> Read After Read
+    WAR,   ///> Write After Read
+    CTRL,  ///> Dependencies related to PHIs or Terminators


what does CTRL stand for?

Ah it stands for CONTROL, I will update the comment.

tmsri

LGTM with some small changes.

tmsri · 2024-10-08T20:04:09Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+      return DependencyType::WAR;
+    if (ToI->mayReadFromMemory())
+      return DependencyType::RAR;
+  }


This is fine, a comment that the order matters in the second case would resolve it.

tmsri · 2024-10-08T21:06:56Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

-    auto *N = getOrCreateNode(I);
-    N->addMemPred(LastMemN);
+  for (Instruction &I : drop_begin(InstrInterval)) {
+    auto *N = getOrCreateNode(&I);
    // Build the Mem node chain.
    if (auto *MemN = dyn_cast<MemDGNode>(N)) {
      MemN->setPrevNode(LastMemN);


Move this into the if condition that follows.

Hmm yeah that would make it less confusing. I will do that in a separate patch, since we are not touching this code in this patch.

tmsri · 2024-10-08T21:25:28Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+  for (MemDGNode &DstN : DstRange) {
+    auto SrcRange =
+        DstRange.getSingleDiff(Interval<MemDGNode>(&DstN, DstRange.bottom()));
+    scanAndAddDeps(DstN, SrcRange);


Why is this not just :
scanAndAddDeps(DstN, Interval(DstN->getNextNode(), DstRange.bottom());

SrcRange spans the MemNodes from DstRange.top() to the one before DstN, which is [DstRange.top(), DstN->getPrevNode()]. So yeah we could just replace this with:

scanAndAddDeps(Dst, Interval(DstRange.top(), DstN->getPrevNode())`;

Let me make this change.

tmsri · 2024-10-08T21:28:21Z

llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp

+    const auto &Preds = DstN->memPreds();
+    auto It = find(Preds, SrcN);
+    return It != Preds.end();
+  }


Why not add this as a member function to DGNode?

We actually have one: hasMemPred(), let me use that instead.

Well, a follow-up patch is refactoring these, so I will leave it as is for now.

llvm/unittests/Transforms/Vectorize/SandboxVectorizer/DependencyGraphTest.cpp

This patch implements actual dependencies checking using BatchAA. This adds memory dep edges between MemDGNodes.

aeubanks · 2024-10-08T22:38:45Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+  // TODO: Check AABudget
+  ModRefInfo SrcModRef =
+      isOrdered(SrcI)
+          ? ModRefInfo::Mod


be conservative with ModRef?

Hmm what do you mean ?

aeubanks · 2024-10-08T23:01:14Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

+  std::unique_ptr<BatchAAResults> BatchAA;
+
+  enum class DependencyType {
+    RAW,   ///> Read After Write


read after write is more annoying to process than write before read, can we use WBR (or something similar) as opposed to RAW?

also maybe we should spell these out instead of using abbreviations? I mostly ask for consistency with other enum classes that are camel case instead of all uppercase (e.g. None instead of NONE)

The reason I am using RAW and not WBR is because it's standard dependency terminology. I agree that WBR is easier to process, but let's stick to what's expected.

Yeah, these uppercase ones need to go. I will update them.

aeubanks · 2024-10-08T23:03:37Z

llvm/include/llvm/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.h

+  enum class DependencyType {
+    RAW,   ///> Read After Write
+    WAW,   ///> Write After Write
+    RAR,   ///> Read After Read


can we group RAR into None?

We could, but None has a different meaning, that the instructions are not accessing memory.

aeubanks · 2024-10-08T23:14:52Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

+      DstN.addMemPred(&SrcN);
+  }
+}
+
 Interval<Instruction> DependencyGraph::extend(ArrayRef<Instruction *> Instrs) {


do we want to assert that the new instructions are not in the DAG?

No, Instrs can overlap with the existing DAG. This will become clear in a follow-up patch.

aeubanks · 2024-10-08T23:19:58Z

llvm/lib/Transforms/Vectorize/SandboxVectorizer/DependencyGraph.cpp

-  return Interval;
+  // Create the dependencies.
+  auto DstRange = MemDGNodeIntervalBuilder::make(InstrInterval, *this);
+  for (MemDGNode &DstN : drop_begin(DstRange)) {


can DstRange be empty?

Yes, I think it could be.

vporpo requested review from slackito, alinas, tschuett, aeubanks, tmsri and Sterling-Augustine October 4, 2024 03:46

llvmbot added vectorization llvm:transforms labels Oct 4, 2024

vporpo force-pushed the DAG branch from 35e3b53 to 80e4ae8 Compare October 4, 2024 16:05

tmsri reviewed Oct 8, 2024

View reviewed changes

vporpo force-pushed the DAG branch from 80e4ae8 to 73bbf98 Compare October 8, 2024 15:51

aeubanks reviewed Oct 8, 2024

View reviewed changes

vporpo force-pushed the DAG branch from 73bbf98 to c741888 Compare October 8, 2024 18:34

tmsri approved these changes Oct 8, 2024

View reviewed changes

[SandboxVec][DAG] Build actual dependencies

09c0ff4

This patch implements actual dependencies checking using BatchAA. This adds memory dep edges between MemDGNodes.

vporpo force-pushed the DAG branch from b0eb027 to 09c0ff4 Compare October 8, 2024 22:45

vporpo merged commit 04a8bff into llvm:main Oct 8, 2024
6 of 8 checks passed

aeubanks reviewed Oct 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SandboxVec][DAG] Build actual dependencies #111094

[SandboxVec][DAG] Build actual dependencies #111094

vporpo commented Oct 4, 2024

llvmbot commented Oct 4, 2024

vporpo commented Oct 4, 2024

tmsri left a comment

tmsri Oct 8, 2024

vporpo Oct 8, 2024

tmsri Oct 8, 2024

vporpo Oct 8, 2024

tmsri Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024 •

edited

Loading

vporpo Oct 8, 2024

tmsri left a comment

tmsri Oct 8, 2024

tmsri Oct 8, 2024

vporpo Oct 8, 2024

tmsri Oct 8, 2024

vporpo Oct 8, 2024

tmsri Oct 8, 2024

vporpo Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

aeubanks Oct 8, 2024

vporpo Oct 8, 2024

[SandboxVec][DAG] Build actual dependencies #111094

[SandboxVec][DAG] Build actual dependencies #111094

Conversation

vporpo commented Oct 4, 2024

llvmbot commented Oct 4, 2024

vporpo commented Oct 4, 2024

tmsri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmsri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks Oct 8, 2024 •

edited

Loading