Skip to content

Reapply "[CodeGen] Introduce MachineDomTreeUpdater" (#96846) #96851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Jun 27, 2024

This reverts commit 0f88493.
Resolve conflict in MachinePostDominators.h There is a conflict after merging #96378, resolved in #96852. Both PRs modified MachinePostDominators.h and triggered build failure.

@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2024

@llvm/pr-subscribers-llvm-analysis

Author: None (paperchalice)

Changes

This reverts commit 0f88493.
Resolve conflict in MachinePostDominators.h


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

11 Files Affected:

  • (modified) llvm/include/llvm/Analysis/DomTreeUpdater.h (+32-164)
  • (added) llvm/include/llvm/Analysis/GenericDomTreeUpdater.h (+525)
  • (modified) llvm/include/llvm/CodeGen/MachineBasicBlock.h (+3)
  • (added) llvm/include/llvm/CodeGen/MachineDomTreeUpdater.h (+72)
  • (modified) llvm/include/llvm/CodeGen/MachinePostDominators.h (+1-1)
  • (modified) llvm/lib/Analysis/DomTreeUpdater.cpp (+2-346)
  • (modified) llvm/lib/CodeGen/CMakeLists.txt (+1)
  • (modified) llvm/lib/CodeGen/MachineBasicBlock.cpp (+6)
  • (added) llvm/lib/CodeGen/MachineDomTreeUpdater.cpp (+62)
  • (modified) llvm/unittests/CodeGen/CMakeLists.txt (+1)
  • (added) llvm/unittests/CodeGen/MachineDomTreeUpdaterTest.cpp (+276)
diff --git a/llvm/include/llvm/Analysis/DomTreeUpdater.h b/llvm/include/llvm/Analysis/DomTreeUpdater.h
index ddb958455ccd7..2b838a311440e 100644
--- a/llvm/include/llvm/Analysis/DomTreeUpdater.h
+++ b/llvm/include/llvm/Analysis/DomTreeUpdater.h
@@ -15,6 +15,8 @@
 #define LLVM_ANALYSIS_DOMTREEUPDATER_H
 
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/Analysis/GenericDomTreeUpdater.h"
+#include "llvm/Analysis/PostDominators.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Support/Compiler.h"
@@ -23,66 +25,17 @@
 #include <vector>
 
 namespace llvm {
-class PostDominatorTree;
 
-class DomTreeUpdater {
-public:
-  enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
-
-  explicit DomTreeUpdater(UpdateStrategy Strategy_) : Strategy(Strategy_) {}
-  DomTreeUpdater(DominatorTree &DT_, UpdateStrategy Strategy_)
-      : DT(&DT_), Strategy(Strategy_) {}
-  DomTreeUpdater(DominatorTree *DT_, UpdateStrategy Strategy_)
-      : DT(DT_), Strategy(Strategy_) {}
-  DomTreeUpdater(PostDominatorTree &PDT_, UpdateStrategy Strategy_)
-      : PDT(&PDT_), Strategy(Strategy_) {}
-  DomTreeUpdater(PostDominatorTree *PDT_, UpdateStrategy Strategy_)
-      : PDT(PDT_), Strategy(Strategy_) {}
-  DomTreeUpdater(DominatorTree &DT_, PostDominatorTree &PDT_,
-                 UpdateStrategy Strategy_)
-      : DT(&DT_), PDT(&PDT_), Strategy(Strategy_) {}
-  DomTreeUpdater(DominatorTree *DT_, PostDominatorTree *PDT_,
-                 UpdateStrategy Strategy_)
-      : DT(DT_), PDT(PDT_), Strategy(Strategy_) {}
-
-  ~DomTreeUpdater() { flush(); }
-
-  /// Returns true if the current strategy is Lazy.
-  bool isLazy() const { return Strategy == UpdateStrategy::Lazy; };
-
-  /// Returns true if the current strategy is Eager.
-  bool isEager() const { return Strategy == UpdateStrategy::Eager; };
-
-  /// Returns true if it holds a DominatorTree.
-  bool hasDomTree() const { return DT != nullptr; }
-
-  /// Returns true if it holds a PostDominatorTree.
-  bool hasPostDomTree() const { return PDT != nullptr; }
-
-  /// Returns true if there is BasicBlock awaiting deletion.
-  /// The deletion will only happen until a flush event and
-  /// all available trees are up-to-date.
-  /// Returns false under Eager UpdateStrategy.
-  bool hasPendingDeletedBB() const { return !DeletedBBs.empty(); }
-
-  /// Returns true if DelBB is awaiting deletion.
-  /// Returns false under Eager UpdateStrategy.
-  bool isBBPendingDeletion(BasicBlock *DelBB) const;
-
-  /// Returns true if either of DT or PDT is valid and the tree has at
-  /// least one update pending. If DT or PDT is nullptr it is treated
-  /// as having no pending updates. This function does not check
-  /// whether there is BasicBlock awaiting deletion.
-  /// Returns false under Eager UpdateStrategy.
-  bool hasPendingUpdates() const;
+class DomTreeUpdater
+    : public GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
+                                   PostDominatorTree> {
+  friend GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
+                               PostDominatorTree>;
 
-  /// Returns true if there are DominatorTree updates queued.
-  /// Returns false under Eager UpdateStrategy or DT is nullptr.
-  bool hasPendingDomTreeUpdates() const;
-
-  /// Returns true if there are PostDominatorTree updates queued.
-  /// Returns false under Eager UpdateStrategy or PDT is nullptr.
-  bool hasPendingPostDomTreeUpdates() const;
+public:
+  using Base =
+      GenericDomTreeUpdater<DomTreeUpdater, DominatorTree, PostDominatorTree>;
+  using Base::Base;
 
   ///@{
   /// \name Mutation APIs
@@ -105,51 +58,6 @@ class DomTreeUpdater {
   /// Although GenericDomTree provides several update primitives,
   /// it is not encouraged to use these APIs directly.
 
-  /// Submit updates to all available trees.
-  /// The Eager Strategy flushes updates immediately while the Lazy Strategy
-  /// queues the updates.
-  ///
-  /// Note: The "existence" of an edge in a CFG refers to the CFG which DTU is
-  /// in sync with + all updates before that single update.
-  ///
-  /// CAUTION!
-  /// 1. It is required for the state of the LLVM IR to be updated
-  /// *before* submitting the updates because the internal update routine will
-  /// analyze the current state of the CFG to determine whether an update
-  /// is valid.
-  /// 2. It is illegal to submit any update that has already been submitted,
-  /// i.e., you are supposed not to insert an existent edge or delete a
-  /// nonexistent edge.
-  void applyUpdates(ArrayRef<DominatorTree::UpdateType> Updates);
-
-  /// Submit updates to all available trees. It will also
-  /// 1. discard duplicated updates,
-  /// 2. remove invalid updates. (Invalid updates means deletion of an edge that
-  /// still exists or insertion of an edge that does not exist.)
-  /// The Eager Strategy flushes updates immediately while the Lazy Strategy
-  /// queues the updates.
-  ///
-  /// Note: The "existence" of an edge in a CFG refers to the CFG which DTU is
-  /// in sync with + all updates before that single update.
-  ///
-  /// CAUTION!
-  /// 1. It is required for the state of the LLVM IR to be updated
-  /// *before* submitting the updates because the internal update routine will
-  /// analyze the current state of the CFG to determine whether an update
-  /// is valid.
-  /// 2. It is illegal to submit any update that has already been submitted,
-  /// i.e., you are supposed not to insert an existent edge or delete a
-  /// nonexistent edge.
-  /// 3. It is only legal to submit updates to an edge in the order CFG changes
-  /// are made. The order you submit updates on different edges is not
-  /// restricted.
-  void applyUpdatesPermissive(ArrayRef<DominatorTree::UpdateType> Updates);
-
-  /// Notify DTU that the entry block was replaced.
-  /// Recalculate all available trees and flush all BasicBlocks
-  /// awaiting deletion immediately.
-  void recalculate(Function &F);
-
   /// Delete DelBB. DelBB will be removed from its Parent and
   /// erased from available trees if it exists and finally get deleted.
   /// Under Eager UpdateStrategy, DelBB will be processed immediately.
@@ -172,33 +80,6 @@ class DomTreeUpdater {
 
   ///@}
 
-  ///@{
-  /// \name Flush APIs
-  ///
-  /// CAUTION! By the moment these flush APIs are called, the current CFG needs
-  /// to be the same as the CFG which DTU is in sync with + all updates
-  /// submitted.
-
-  /// Flush DomTree updates and return DomTree.
-  /// It flushes Deleted BBs if both trees are up-to-date.
-  /// It must only be called when it has a DomTree.
-  DominatorTree &getDomTree();
-
-  /// Flush PostDomTree updates and return PostDomTree.
-  /// It flushes Deleted BBs if both trees are up-to-date.
-  /// It must only be called when it has a PostDomTree.
-  PostDominatorTree &getPostDomTree();
-
-  /// Apply all pending updates to available trees and flush all BasicBlocks
-  /// awaiting deletion.
-
-  void flush();
-
-  ///@}
-
-  /// Debug method to help view the internal state of this class.
-  LLVM_DUMP_METHOD void dump() const;
-
 private:
   class CallBackOnDeletion final : public CallbackVH {
   public:
@@ -216,16 +97,7 @@ class DomTreeUpdater {
     }
   };
 
-  SmallVector<DominatorTree::UpdateType, 16> PendUpdates;
-  size_t PendDTUpdateIndex = 0;
-  size_t PendPDTUpdateIndex = 0;
-  DominatorTree *DT = nullptr;
-  PostDominatorTree *PDT = nullptr;
-  const UpdateStrategy Strategy;
-  SmallPtrSet<BasicBlock *, 8> DeletedBBs;
   std::vector<CallBackOnDeletion> Callbacks;
-  bool IsRecalculatingDomTree = false;
-  bool IsRecalculatingPostDomTree = false;
 
   /// First remove all the instructions of DelBB and then make sure DelBB has a
   /// valid terminator instruction which is necessary to have when DelBB still
@@ -237,32 +109,28 @@ class DomTreeUpdater {
   /// Returns true if at least one BasicBlock is deleted.
   bool forceFlushDeletedBB();
 
-  /// Helper function to apply all pending DomTree updates.
-  void applyDomTreeUpdates();
-
-  /// Helper function to apply all pending PostDomTree updates.
-  void applyPostDomTreeUpdates();
-
-  /// Helper function to flush deleted BasicBlocks if all available
-  /// trees are up-to-date.
-  void tryFlushDeletedBB();
-
-  /// Drop all updates applied by all available trees and delete BasicBlocks if
-  /// all available trees are up-to-date.
-  void dropOutOfDateUpdates();
-
-  /// Erase Basic Block node that has been unlinked from Function
-  /// in the DomTree and PostDomTree.
-  void eraseDelBBNode(BasicBlock *DelBB);
-
-  /// Returns true if the update appears in the LLVM IR.
-  /// It is used to check whether an update is valid in
-  /// insertEdge/deleteEdge or is unnecessary in the batch update.
-  bool isUpdateValid(DominatorTree::UpdateType Update) const;
-
-  /// Returns true if the update is self dominance.
-  bool isSelfDominance(DominatorTree::UpdateType Update) const;
+  /// Debug method to help view the internal state of this class.
+  LLVM_DUMP_METHOD void dump() const {
+    Base::dump();
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+    raw_ostream &OS = dbgs();
+    OS << "Pending Callbacks:\n";
+    int Index = 0;
+    for (const auto &BB : Callbacks) {
+      OS << "  " << Index << " : ";
+      ++Index;
+      if (BB->hasName())
+        OS << BB->getName() << "(";
+      else
+        OS << "(no_name)(";
+      OS << BB << ")\n";
+    }
+#endif
+  }
 };
+
+extern template class GenericDomTreeUpdater<DomTreeUpdater, DominatorTree,
+                                            PostDominatorTree>;
 } // namespace llvm
 
 #endif // LLVM_ANALYSIS_DOMTREEUPDATER_H
diff --git a/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
new file mode 100644
index 0000000000000..7092c67083a67
--- /dev/null
+++ b/llvm/include/llvm/Analysis/GenericDomTreeUpdater.h
@@ -0,0 +1,525 @@
+//===- GenericDomTreeUpdater.h ----------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the GenericDomTreeUpdater class, which provides a uniform
+// way to update dominator tree related data structures.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ANALYSIS_GENERICDOMTREEUPDATER_H
+#define LLVM_ANALYSIS_GENERICDOMTREEUPDATER_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/SmallSet.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/raw_ostream.h"
+
+namespace llvm {
+
+template <typename DerivedT, typename DomTreeT, typename PostDomTreeT>
+class GenericDomTreeUpdater {
+  DerivedT &derived() { return *static_cast<DerivedT *>(this); }
+  const DerivedT &derived() const {
+    return *static_cast<const DerivedT *>(this);
+  }
+
+public:
+  enum class UpdateStrategy : unsigned char { Eager = 0, Lazy = 1 };
+  using BasicBlockT = typename DomTreeT::NodeType;
+
+  explicit GenericDomTreeUpdater(UpdateStrategy Strategy_)
+      : Strategy(Strategy_) {}
+  GenericDomTreeUpdater(DomTreeT &DT_, UpdateStrategy Strategy_)
+      : DT(&DT_), Strategy(Strategy_) {}
+  GenericDomTreeUpdater(DomTreeT *DT_, UpdateStrategy Strategy_)
+      : DT(DT_), Strategy(Strategy_) {}
+  GenericDomTreeUpdater(PostDomTreeT &PDT_, UpdateStrategy Strategy_)
+      : PDT(&PDT_), Strategy(Strategy_) {}
+  GenericDomTreeUpdater(PostDomTreeT *PDT_, UpdateStrategy Strategy_)
+      : PDT(PDT_), Strategy(Strategy_) {}
+  GenericDomTreeUpdater(DomTreeT &DT_, PostDomTreeT &PDT_,
+                        UpdateStrategy Strategy_)
+      : DT(&DT_), PDT(&PDT_), Strategy(Strategy_) {}
+  GenericDomTreeUpdater(DomTreeT *DT_, PostDomTreeT *PDT_,
+                        UpdateStrategy Strategy_)
+      : DT(DT_), PDT(PDT_), Strategy(Strategy_) {}
+
+  ~GenericDomTreeUpdater() { flush(); }
+
+  /// Returns true if the current strategy is Lazy.
+  bool isLazy() const { return Strategy == UpdateStrategy::Lazy; };
+
+  /// Returns true if the current strategy is Eager.
+  bool isEager() const { return Strategy == UpdateStrategy::Eager; };
+
+  /// Returns true if it holds a DomTreeT.
+  bool hasDomTree() const { return DT != nullptr; }
+
+  /// Returns true if it holds a PostDomTreeT.
+  bool hasPostDomTree() const { return PDT != nullptr; }
+
+  /// Returns true if there is BasicBlockT awaiting deletion.
+  /// The deletion will only happen until a flush event and
+  /// all available trees are up-to-date.
+  /// Returns false under Eager UpdateStrategy.
+  bool hasPendingDeletedBB() const { return !DeletedBBs.empty(); }
+
+  /// Returns true if DelBB is awaiting deletion.
+  /// Returns false under Eager UpdateStrategy.
+  bool isBBPendingDeletion(BasicBlockT *DelBB) const {
+    if (Strategy == UpdateStrategy::Eager || DeletedBBs.empty())
+      return false;
+    return DeletedBBs.contains(DelBB);
+  }
+
+  /// Returns true if either of DT or PDT is valid and the tree has at
+  /// least one update pending. If DT or PDT is nullptr it is treated
+  /// as having no pending updates. This function does not check
+  /// whether there is MachineBasicBlock awaiting deletion.
+  /// Returns false under Eager UpdateStrategy.
+  bool hasPendingUpdates() const {
+    return hasPendingDomTreeUpdates() || hasPendingPostDomTreeUpdates();
+  }
+
+  /// Returns true if there are DomTreeT updates queued.
+  /// Returns false under Eager UpdateStrategy or DT is nullptr.
+  bool hasPendingDomTreeUpdates() const {
+    if (!DT)
+      return false;
+    return PendUpdates.size() != PendDTUpdateIndex;
+  }
+
+  /// Returns true if there are PostDomTreeT updates queued.
+  /// Returns false under Eager UpdateStrategy or PDT is nullptr.
+  bool hasPendingPostDomTreeUpdates() const {
+    if (!PDT)
+      return false;
+    return PendUpdates.size() != PendPDTUpdateIndex;
+  }
+
+  ///@{
+  /// \name Mutation APIs
+  ///
+  /// These methods provide APIs for submitting updates to the DomTreeT and
+  /// the PostDominatorTree.
+  ///
+  /// Note: There are two strategies to update the DomTreeT and the
+  /// PostDominatorTree:
+  /// 1. Eager UpdateStrategy: Updates are submitted and then flushed
+  /// immediately.
+  /// 2. Lazy UpdateStrategy: Updates are submitted but only flushed when you
+  /// explicitly call Flush APIs. It is recommended to use this update strategy
+  /// when you submit a bunch of updates multiple times which can then
+  /// add up to a large number of updates between two queries on the
+  /// DomTreeT. The incremental updater can reschedule the updates or
+  /// decide to recalculate the dominator tree in order to speedup the updating
+  /// process depending on the number of updates.
+  ///
+  /// Although GenericDomTree provides several update primitives,
+  /// it is not encouraged to use these APIs directly.
+
+  /// Notify DTU that the entry block was replaced.
+  /// Recalculate all available trees and flush all BasicBlocks
+  /// awaiting deletion immediately.
+  template <typename FuncT> void recalculate(FuncT &F) {
+    if (Strategy == UpdateStrategy::Eager) {
+      if (DT)
+        DT->recalculate(F);
+      if (PDT)
+        PDT->recalculate(F);
+      return;
+    }
+
+    // There is little performance gain if we pend the recalculation under
+    // Lazy UpdateStrategy so we recalculate available trees immediately.
+
+    // Prevent forceFlushDeletedBB() from erasing DomTree or PostDomTree nodes.
+    IsRecalculatingDomTree = IsRecalculatingPostDomTree = true;
+
+    // Because all trees are going to be up-to-date after recalculation,
+    // flush awaiting deleted BasicBlocks.
+    derived().forceFlushDeletedBB();
+    if (DT)
+      DT->recalculate(F);
+    if (PDT)
+      PDT->recalculate(F);
+
+    // Resume forceFlushDeletedBB() to erase DomTree or PostDomTree nodes.
+    IsRecalculatingDomTree = IsRecalculatingPostDomTree = false;
+    PendDTUpdateIndex = PendPDTUpdateIndex = PendUpdates.size();
+    dropOutOfDateUpdates();
+  }
+
+  /// Submit updates to all available trees.
+  /// The Eager Strategy flushes updates immediately while the Lazy Strategy
+  /// queues the updates.
+  ///
+  /// Note: The "existence" of an edge in a CFG refers to the CFG which DTU is
+  /// in sync with + all updates before that single update.
+  ///
+  /// CAUTION!
+  /// 1. It is required for the state of the LLVM IR to be updated
+  /// *before* submitting the updates because the internal update routine will
+  /// analyze the current state of the CFG to determine whether an update
+  /// is valid.
+  /// 2. It is illegal to submit any update that has already been submitted,
+  /// i.e., you are supposed not to insert an existent edge or delete a
+  /// nonexistent edge.
+  void applyUpdates(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    if (!DT && !PDT)
+      return;
+
+    if (Strategy == UpdateStrategy::Lazy) {
+      PendUpdates.reserve(PendUpdates.size() + Updates.size());
+      for (const auto &U : Updates)
+        if (!isSelfDominance(U))
+          PendUpdates.push_back(U);
+
+      return;
+    }
+
+    if (DT)
+      DT->applyUpdates(Updates);
+    if (PDT)
+      PDT->applyUpdates(Updates);
+  }
+
+  /// Submit updates to all available trees. It will also
+  /// 1. discard duplicated updates,
+  /// 2. remove invalid updates. (Invalid updates means deletion of an edge that
+  /// still exists or insertion of an edge that does not exist.)
+  /// The Eager Strategy flushes updates immediately while the Lazy Strategy
+  /// queues the updates.
+  ///
+  /// Note: The "existence" of an edge in a CFG refers to the CFG which DTU is
+  /// in sync with + all updates before that single update.
+  ///
+  /// CAUTION!
+  /// 1. It is required for the state of the LLVM IR to be updated
+  /// *before* submitting the updates because the internal update routine will
+  /// analyze the current state of the CFG to determine whether an update
+  /// is valid.
+  /// 2. It is illegal to submit any update that has already been submitted,
+  /// i.e., you are supposed not to insert an existent edge or delete a
+  /// nonexistent edge.
+  /// 3. It is only legal to submit updates to an edge in the order CFG changes
+  /// are made. The order you submit updates on different edges is not
+  /// restricted.
+  void applyUpdatesPermissive(ArrayRef<typename DomTreeT::UpdateType> Updates) {
+    if (!DT && !PDT)
+      return;
+
+    SmallSet<std::pair<BasicBlockT *, BasicBlockT *>, 8> Seen;
+    SmallVector<typename DomTreeT::UpdateType, 8> DeduplicatedUpdates;
+    for (const auto &U : Updates) {
+      auto Edge = std::make_pair(U.getFrom(), U.getTo());
+      // Because it is illegal to submit updates that have already been applied
+      // and updates to an edge need to be strictly ordered,
+      // it is safe to infer the existence of an edge from the first update
+      // to this edge.
+      // If the first update to an edge is "Delete", it means that the edge
+      // existed before. If the first update to an edge is "Insert", it means
+      // that the edge didn't exist before.
+      //
+      // For example, if the user submits {{Delete, A, B}, {Insert, A, B}},
+      // because
+      // 1. it is illegal to submit updates that have already been applied,
+      // i.e., user cannot delete an nonexistent edge,
+      // 2. updates to an edge need to be strictly ordered,
+      // So, initially edge A -> B existed.
+      // We can then safely ignore future updates to this edge and directly
+      // inspect the current CFG:
+      // a. If the edge still exists, because the user cannot insert an existent
+      // edge, so both {Delete, A, B}, {Insert, A, B} actually happened and
+      // resulted in a no-op. DTU won't submit any update in this case.
+      // b. If the edge doesn't exist, we can then infer that {Delete, A, B}
+      // actually happened but {Insert, A, B} was an invalid update which never
+      // happened. DTU will submit {Delete, A, B} in this case.
+      if (!isSelfDominance(U) && Seen.count(Edge) == 0) {
+        Seen.insert(Edge);
+        // If the update doesn't appear in the CFG, it means that
+        // either the change isn't made or relevant operat...
[truncated]

@@ -0,0 +1,72 @@
//===- llvm/CodeGen/MachineDomTreeUpdater.h -----------------------*- C++
//-*-==//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad line break

This reverts commit 0f88493.
Resolve conflict in `MachinePostDominators.h`
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what the issues were with the initial PR that caused it to fail and how this PR fixes these? I'd expect this in the PR description.

@paperchalice paperchalice merged commit c931ac5 into llvm:main Jun 28, 2024
7 checks passed
@paperchalice paperchalice deleted the dtu branch June 28, 2024 07:11
@nikic
Copy link
Contributor

nikic commented Jun 28, 2024

This change has effectively moved DomTreeUpdater.cpp into DomTreeUpdator.h. Please rework this so that GenericDomTreeUpdater is split into two files, GenericDomTreeUpdater.h providing the interface, and GenericDomTreeUpdaterImpl.h providing the implementation that is only included in the file that explicitly instantiates the template.

(This should also allow you to drop the PostDominators.h include in the header.)

@thurstond
Copy link
Contributor

I think this broke the build, starting at https://lab.llvm.org/buildbot/#/builders/169/builds/490

It's still broken after aca71ef: https://lab.llvm.org/buildbot/#/builders/169/builds/501

==1313502==ERROR: AddressSanitizer: use-after-poison on address 0x521000012338 at pc 0x555820abcc52 bp 0x7ffc4ba96960 sp 0x7ffc4ba96958
READ of size 8 at 0x521000012338 thread T0
    #0 0x555820abcc51 in asInt /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:41:5
    #1 0x555820abcc51 in operator long /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:45:48
    #2 0x555820abcc51 in getInt /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:96:57
    #3 0x555820abcc51 in isSentinel /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:44:52
    #4 0x555820abcc51 in isKnownSentinel /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:45:41
    #5 0x555820abcc51 in llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::MachineBasicBlock, true, false, void, false, void>, false, false>::operator*() const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:168:5
    #6 0x555820abe4e7 in MachineDomTreeUpdaterTest_LazyUpdateBasicOperations_Test::TestBody() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/CodeGen/MachineDomTreeUpdaterTest.cpp:275:3

@paperchalice
Copy link
Contributor Author

I think this broke the build, starting at https://lab.llvm.org/buildbot/#/builders/169/builds/490

It's still broken after aca71ef: https://lab.llvm.org/buildbot/#/builders/169/builds/501

==1313502==ERROR: AddressSanitizer: use-after-poison on address 0x521000012338 at pc 0x555820abcc52 bp 0x7ffc4ba96960 sp 0x7ffc4ba96958
READ of size 8 at 0x521000012338 thread T0
    #0 0x555820abcc51 in asInt /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:41:5
    #1 0x555820abcc51 in operator long /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:45:48
    #2 0x555820abcc51 in getInt /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:96:57
    #3 0x555820abcc51 in isSentinel /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:44:52
    #4 0x555820abcc51 in isKnownSentinel /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:45:41
    #5 0x555820abcc51 in llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::MachineBasicBlock, true, false, void, false, void>, false, false>::operator*() const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:168:5
    #6 0x555820abe4e7 in MachineDomTreeUpdaterTest_LazyUpdateBasicOperations_Test::TestBody() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/unittests/CodeGen/MachineDomTreeUpdaterTest.cpp:275:3

Preparing fix now.

paperchalice added a commit that referenced this pull request Jun 29, 2024
In #96851, the unit test contains use after free, which triggers
sanitizer error.
Fix https://lab.llvm.org/buildbot/#/builders/169/builds/490
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
…lvm#96851)

This reverts commit 0f88493.
Resolve conflict in `MachinePostDominators.h` There is a conflict after
merging llvm#96378, resolved in llvm#96852. Both PRs modified
`MachinePostDominators.h` and triggered build failure.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants