[Mlir-commits] [mlir] 3077c13 - [mlir][NFC] Make InsertionGuard properly move constructible

[Markus Böck]

Author: Markus Böck
Date: 2021-10-29T08:56:49+02:00
New Revision: 3077c13f91ef93bb5c1c836b167c8203fca9c26b

URL: https://github.com/llvm/llvm-project/commit/3077c13f91ef93bb5c1c836b167c8203fca9c26b
DIFF: https://github.com/llvm/llvm-project/commit/3077c13f91ef93bb5c1c836b167c8203fca9c26b.diff

LOG: [mlir][NFC] Make InsertionGuard properly move constructible

InsertionGuards move constructor is currently the compiler synthesized implementation which is very bug prone. A move constructed InsertionGuard will get the same builder and insertion point as the one it is constructed from, leading to insertion point being restored twice. This can even happen in non obvious situations on some compilers, such as when returning a move constructible struct from a function.

This patch fixes the issue by properly implementing the move constructor. An InsertionGuard that was used to move construct another InsertionGuard is simply inactive and will not restore the insertion point.

I chose to explicitly delete the move assign operator as its semantics are not clear cut. If one were to strictly follow the rule of 5, you'd have to restore the insertion point before then taking ownership of the others guards fields. I believe that to be rather confusing and/or surprising however. One may still get such semantics using llvm::Optional or std::optional and the emplace method if really needed.

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

Added: 
    

Modified: 
    mlir/include/mlir/IR/Builders.h

Removed: 
    


################################################################################
diff  --git a/mlir/include/mlir/IR/Builders.h b/mlir/include/mlir/IR/Builders.h
index 6a50bc782f6e4..72b9ca48d79c9 100644
--- a/mlir/include/mlir/IR/Builders.h
+++ b/mlir/include/mlir/IR/Builders.h
@@ -281,11 +281,28 @@ class OpBuilder : public Builder {
   class InsertionGuard {
   public:
     InsertionGuard(OpBuilder &builder)
-        : builder(builder), ip(builder.saveInsertionPoint()) {}
-    ~InsertionGuard() { builder.restoreInsertionPoint(ip); }
+        : builder(&builder), ip(builder.saveInsertionPoint()) {}
+
+    ~InsertionGuard() {
+      if (builder)
+        builder->restoreInsertionPoint(ip);
+    }
+
+    InsertionGuard(const InsertionGuard &) = delete;
+    InsertionGuard &operator=(const InsertionGuard &) = delete;
+
+    /// Implement the move constructor to clear the builder field of `other`.
+    /// That way it does not restore the insertion point upon destruction as
+    /// that should be done exclusively by the just constructed InsertionGuard.
+    InsertionGuard(InsertionGuard &&other) noexcept
+        : builder(other.builder), ip(other.ip) {
+      other.builder = nullptr;
+    }
+
+    InsertionGuard &operator=(InsertionGuard &&other) = delete;
 
   private:
-    OpBuilder &builder;
+    OpBuilder *builder;
     OpBuilder::InsertPoint ip;
   };