PR cleanup

Author: mbagherbeikTT

mlir/include/mlir/Interfaces/SideEffectInterfaces.h

@@ -347,12 +347,18 @@ struct AlwaysSpeculatableImplTrait
 //===----------------------------------------------------------------------===//
 
 namespace MemoryEffects {
+/// Defines the priority of the different memory effects.
+/// 
+/// Sorting/ordering memory effects of an operation is done based on
+/// their defined stage and priority, in that order. If stage values for two effect instances are
+/// equal, they are then sorted by priority. Lower priority values indicate higher
+/// precedence.
 enum Priority {
-  kDefaultPriority = 0,
-  kAllocPriority = 1,
-  kFreePriority = 2,
-  kReadPriority = 3,
-  kWritePriority = 4
+  DefaultPriority = 0,
+  AllocPriority = 1,
+  FreePriority = 2,
+  ReadPriority = 3,
+  WritePriority = 4
 };
 
 /// This class represents the base class used for memory effects.
@@ -370,41 +376,41 @@ struct Effect : public SideEffects::Effect {
 
 protected:
   /// Priority value for this effect. Lower numbers indicate higher precedence.
-  Priority priority = Priority::kDefaultPriority;
+  Priority priority = Priority::DefaultPriority;
 };
 using EffectInstance = SideEffects::EffectInstance<Effect>;
 
-/// Returns vector of effects sorted by effect stage then priority
-/// priority order: allocate -> free -> read -> write
+/// Returns vector of the op's memory effects sorted in increasing stage order
+/// and in increasing priority order within each stage.
 llvm::SmallVector<MemoryEffects::EffectInstance>
 getMemoryEffectsSorted(Operation *op);
 
 /// The following effect indicates that the operation allocates from some
 /// resource. An 'allocate' effect implies only allocation of the resource, and
 /// not any visible mutation or dereference.
 struct Allocate : public Effect::Base<Allocate> {
-  Allocate() : Effect::Base<Allocate>() { this->priority = Priority::kAllocPriority; }
+  Allocate() : Effect::Base<Allocate>() { this->priority = Priority::AllocPriority; }
 };
 
 /// The following effect indicates that the operation frees some resource that
 /// has been allocated. An 'allocate' effect implies only de-allocation of the
 /// resource, and not any visible allocation, mutation or dereference.
 struct Free : public Effect::Base<Free> {
-  Free() : Effect::Base<Free>() { this->priority = Priority::kFreePriority; }
+  Free() : Effect::Base<Free>() { this->priority = Priority::FreePriority; }
 };
 
 /// The following effect indicates that the operation reads from some resource.
 /// A 'read' effect implies only dereferencing of the resource, and not any
 /// visible mutation.
 struct Read : public Effect::Base<Read> {
-  Read() : Effect::Base<Read>() { this->priority = Priority::kReadPriority; }
+  Read() : Effect::Base<Read>() { this->priority = Priority::ReadPriority; }
 };
 
 /// The following effect indicates that the operation writes to some resource. A
 /// 'write' effect implies only mutating a resource, and not any visible
 /// dereference or read.
 struct Write : public Effect::Base<Write> {
-  Write() : Effect::Base<Write>() { this->priority = Priority::kWritePriority; }
+  Write() : Effect::Base<Write>() { this->priority = Priority::WritePriority; }
 };
 } // namespace MemoryEffects
 

mlir/include/mlir/Transforms/LoopInvariantCodeMotionUtils.h

@@ -23,14 +23,34 @@ class Region;
 class RewriterBase;
 class Value;
 
-/// Gathers potential conflicts on all memory resources used within loop
+/// Gathers potential conflicts on all memory resources used within loop.
 ///
 /// Given a target loop and an op within it (or the loop op itself), 
 /// gathers op's memory effects and flags potential resource conflicts 
 /// in a map and then recurses into the op's regions to gather nested 
-/// resource conflicts 
+/// resource conflicts. 
 ///
+/// Typical usage:
+/// \code
+///   LoopLikeOpInterface myLoop = ...;
+///   DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> myConflicts;
+///   gatherResourceConflicts(myLoop, myLoop.getOperation(), resourceConflicts);
+/// \endcode
+///
+/// \param loop The loop to gather resource conflicts for.
+/// \param op The operation to gather resource conflicts for,
+/// typically the loop op itself via loop.getOperation().
+/// \param resourceConflicts Map to store potential resource conflicts.
+/// Key is the resource ID that effects are applied to. Value is a pair of
+/// a boolean, indicating if the resource has a conflict, and the last effect
+/// that was applied to the resource (if no conflicts exist) or the effect
+/// that caused the conflict (if conflicts exist). This was done so we 
+/// check the effect that causes the conflict for debugging purposes.
 /// First call should use loop = someLoop and op = someLoop.getOperation()
+///
+/// resourceConflicts is modified by the function and will be non-empty
+/// as long as there are memory effects within the loop, even if there are
+/// no conflicts.
 void gatherResourceConflicts(LoopLikeOpInterface loop, Operation *op,
     DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts);
 

mlir/lib/Interfaces/CMakeLists.txt

@@ -85,21 +85,7 @@ add_mlir_interface_library(MemorySlotInterfaces)
 add_mlir_interface_library(ParallelCombiningOpInterface)
 add_mlir_interface_library(RuntimeVerifiableOpInterface)
 add_mlir_interface_library(ShapedOpInterfaces)
-
-add_mlir_library(MLIRSideEffectInterfaces
-  SideEffectInterfaces.cpp
-
-  ADDITIONAL_HEADER_DIRS
-  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Interfaces
-
-  DEPENDS
-  MLIRSideEffectInterfacesIncGen
-
-  LINK_LIBS PUBLIC
-  MLIRIR
-  MLIRDialectUtils
-  MLIRLoopLikeInterface
-)
+add_mlir_interface_library(SideEffectInterfaces)
 
 add_mlir_library(MLIRSubsetOpInterface
   SubsetOpInterface.cpp

mlir/lib/Transforms/Utils/LoopInvariantCodeMotionUtils.cpp

@@ -61,8 +61,15 @@ static bool canBeHoisted(Operation *op,
 }
 
 /// Merges srcEffect's Memory Effect on its resource into the 
-/// resourceConflicts map, flagging resources if the srcEffect
-/// results in a conflict
+/// resourceConflicts map, flagging the resource if the srcEffect
+/// results in a conflict.
+///
+/// \param resourceConflicts The map to store resources' conflicts status.
+/// \param srcEffect The effect to merge into the resourceConflicts map.
+/// \param srcHasConflict Whether the srcEffect results in a conflict based 
+/// on higher level analysis.
+///
+/// resourceConflicts is modified by the function and will be non-empty
 static void mergeResource(
   DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts,
   const MemoryEffects::EffectInstance &srcEffect,
@@ -110,9 +117,9 @@ static bool hasLoopVariantInput(LoopLikeOpInterface loopLike, Operation *op) {
 
 /// Returns true if:
 /// (a) any of the resources used by op's Memory Effects have been
-/// flagged as having a conflict within the resourceConflicts map
+/// flagged as having a conflict within the resourceConflicts map OR
 /// (b) op doesn't have a MemoryEffectOpInterface or has one but
-/// without any specific effects
+/// without any specific effects.
 static bool mayHaveMemoryEffectConflict(Operation *op,
   DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> &resourceConflicts) {
 
@@ -155,7 +162,7 @@ void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
 
   if (auto memInterface = dyn_cast<MemoryEffectOpInterface>(op)) {
     // gather all effects on op
-    llvm::SmallVector<MemoryEffects::EffectInstance> effects =
+    SmallVector<MemoryEffects::EffectInstance> effects =
       MemoryEffects::getMemoryEffectsSorted(op);
 
     if (!effects.empty()) {
@@ -164,6 +171,8 @@ void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
       bool writesConflict = hasLoopVariantInput(loopLike, op);
 
       for (const MemoryEffects::EffectInstance &effect : effects) {
+        LDBG() << "Effect: " << effect.getEffect()->getPriority() << " with resource: " << effect.getResource()->getName() << " op " << op->getName();
+
         bool conflict = false;
         bool isWrite = isa<MemoryEffects::Write>(effect.getEffect());
 
@@ -173,8 +182,7 @@ void mlir::gatherResourceConflicts(LoopLikeOpInterface loopLike, Operation *op,
         if (isa<MemoryEffects::Read>(effect.getEffect()))
           writesConflict = true;
 
-        if (isWrite)
-          if (writesConflict)
+        if (isWrite && writesConflict)
             conflict = true;
 
         mergeResource(resourceConflicts, effect, conflict);
@@ -194,19 +202,25 @@ size_t mlir::moveLoopInvariantCode(
     function_ref<void(Operation *, Region *)> moveOutOfRegion) {
   size_t numMoved = 0;
 
+  // TODO: see if this can be spec'd in a meaningful way to add back later.
+  //
   // check that the loop isn't dead
   // auto isDead = loopLike.isZeroTrip();
   // if (!isDead.has_value() || isDead.value())
   //   return numMoved;
 
-  // go through loop body and map out resource usages
-  // op->regions are essentially merged sequentially
-  // e.g. an if's "then" and "else" regions are treated like one
-  // continuous region --> need to add fork checking
+  // Go through loop body and map out resource usages.
+  // op->regions are essentially merged sequentially.
+  // E.g., an if's "then" and "else" regions are treated like one
+  // continuous region --> need to add fork checking.
   //
-  // loop "do" and "then" regions also merged
+  // loop "do" and "then" regions also merged.
   DenseMap<TypeID, std::pair<bool, MemoryEffects::EffectInstance>> resourceConflicts;
-  mlir::gatherResourceConflicts(loopLike, loopLike.getOperation(), resourceConflicts);
+  gatherResourceConflicts(loopLike, loopLike.getOperation(), resourceConflicts);
+
+  for (auto &item : resourceConflicts) {
+    LDBG() << "Resource: " << item.second.second.getResource()->getName() << " has conflict: " << item.second.first << " with effect: " << item.second.second.getEffect()->getPriority();
+  } 
 
   auto regions = loopLike.getLoopRegions();
   for (Region *region : regions) {
@@ -233,7 +247,13 @@ size_t mlir::moveLoopInvariantCode(
 
       bool noMemoryConflicts = isMemoryEffectFree(op) 
         || !mayHaveMemoryEffectConflict(op, resourceConflicts);
-
+        
+      LDBG() << "isMemoryEffectFree: " << isMemoryEffectFree(op) 
+        << " mayHaveMemoryEffectConflict: " << mayHaveMemoryEffectConflict(op, resourceConflicts)
+        << " noMemoryConflicts: " << noMemoryConflicts
+        << " shouldMoveOutOfRegion: " << shouldMoveOutOfRegion(op, region)
+        << " canBeHoisted: " << canBeHoisted(op, definedOutside);
+        
       if (!noMemoryConflicts
         || !shouldMoveOutOfRegion(op, region)
         || !canBeHoisted(op, definedOutside))

mlir/test/lib/Dialect/Test/TestOps.td

@@ -2978,7 +2978,7 @@ def TestEffectsResult : TEST_Op<"test_effects_result"> {
 }
 
 //===----------------------------------------------------------------------===//
-// Test Ops with multiple effects for Loop Invariant Code Motion
+// Test Ops with multiple effects for Loop Invariant Code Motion.
 //===----------------------------------------------------------------------===//
 
 def TestResourceA : Resource<"TestResourceA">;