[Mlir-commits] [mlir] 8bdbe29 - [mlir] Fix bug in computing operation order

[James Molloy]

Author: James Molloy
Date: 2020-10-09T12:18:52+01:00
New Revision: 8bdbe29519236b0dc6a701deb455440c336f2773

URL: https://github.com/llvm/llvm-project/commit/8bdbe29519236b0dc6a701deb455440c336f2773
DIFF: https://github.com/llvm/llvm-project/commit/8bdbe29519236b0dc6a701deb455440c336f2773.diff

LOG: [mlir] Fix bug in computing operation order

When attempting to compute a differential orderIndex we were calculating the
bailout condition correctly, but then an errant "+ 1" meant the orderIndex we
created was invalid.

Added test.

Reviewed By: ftynse

Differential Revision: https://reviews.llvm.org/D89115

Added: 
    

Modified: 
    mlir/lib/IR/Operation.cpp
    mlir/unittests/IR/OperationSupportTest.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/IR/Operation.cpp b/mlir/lib/IR/Operation.cpp
index f531a6097c25..fa49ccad5d52 100644
--- a/mlir/lib/IR/Operation.cpp
+++ b/mlir/lib/IR/Operation.cpp
@@ -394,7 +394,7 @@ void Operation::updateOrderIfNecessary() {
   // Check to see if there is a valid order between the two.
   if (prevOrder + 1 == nextOrder)
     return block->recomputeOpOrder();
-  orderIndex = prevOrder + 1 + ((nextOrder - prevOrder) / 2);
+  orderIndex = prevOrder + ((nextOrder - prevOrder) / 2);
 }
 
 //===----------------------------------------------------------------------===//

diff  --git a/mlir/unittests/IR/OperationSupportTest.cpp b/mlir/unittests/IR/OperationSupportTest.cpp
index 96693309bd13..3b905c2ac4b2 100644
--- a/mlir/unittests/IR/OperationSupportTest.cpp
+++ b/mlir/unittests/IR/OperationSupportTest.cpp
@@ -16,11 +16,12 @@ using namespace mlir::detail;
 
 static Operation *createOp(MLIRContext *context,
                            ArrayRef<Value> operands = llvm::None,
-                           ArrayRef<Type> resultTypes = llvm::None) {
+                           ArrayRef<Type> resultTypes = llvm::None,
+                           unsigned int numRegions = 0) {
   context->allowUnregisteredDialects();
   return Operation::create(UnknownLoc::get(context),
                            OperationName("foo.bar", context), resultTypes,
-                           operands, llvm::None, llvm::None, 0);
+                           operands, llvm::None, llvm::None, numRegions);
 }
 
 namespace {
@@ -149,4 +150,36 @@ TEST(OperandStorageTest, MutableRange) {
   useOp->destroy();
 }
 
+TEST(OperationOrderTest, OrderIsAlwaysValid) {
+  MLIRContext context(false);
+  Builder builder(&context);
+
+  Operation *containerOp =
+      createOp(&context, /*operands=*/llvm::None, /*resultTypes=*/llvm::None,
+               /*numRegions=*/1);
+  Region &region = containerOp->getRegion(0);
+  Block *block = new Block();
+  region.push_back(block);
+
+  // Insert two operations, then iteratively add more operations in the middle
+  // of them. Eventually we will insert more than kOrderStride operations and
+  // the block order will need to be recomputed.
+  Operation *frontOp = createOp(&context);
+  Operation *backOp = createOp(&context);
+  block->push_back(frontOp);
+  block->push_back(backOp);
+
+  // Chosen to be larger than Operation::kOrderStride.
+  int kNumOpsToInsert = 10;
+  for (int i = 0; i < kNumOpsToInsert; ++i) {
+    Operation *op = createOp(&context);
+    block->getOperations().insert(backOp->getIterator(), op);
+    ASSERT_TRUE(op->isBeforeInBlock(backOp));
+    // Note verifyOpOrder() returns false if the order is valid.
+    ASSERT_FALSE(block->verifyOpOrder());
+  }
+
+  containerOp->destroy();
+}
+
 } // end namespace