[mlir][NFC] Make InsertionGuard properly move constructible

[zero9178]

Summary

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 (https://godbolt.org/z/4j7M98Khq notice the call to ~Test twice, once in test(), the other in main).

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 this to rather confusing/and or surprising however. One may still get such semantics using llvm::Optional or std::optional and the emplace method if you really want it.

[lattner]

Makes sense to me. Is there a reason that we need any move ctor? Why not just disable it?

[rriddle]

In D112749#3094980, @lattner wrote:

Makes sense to me. Is there a reason that we need any move ctor? Why not just disable it?

Yeah, it what situation are you moving the guard?

[zero9178]

In D112749#3094980, @lattner wrote:

Makes sense to me. Is there a reason that we need any move ctor? Why not just disable it?

Without a move constructor one is unable to return an insertion guard from a function and generally makes it harder to compose inside of other classes including things like Optional. None of those things were previously impossible to be fair, but needlessly cumbersome having to always do inplace construction or wrapping the guard in a uniue_ptr.

[lattner]

Huh ok, that makes sense to me. Composition-ality is good.

[mehdi_amini]

LG

mlir/include/mlir/IR/Builders.h

297

Can you document it? Something like:

/// Implement the move constructor so that the builder is cleared from `other`
/// to avoid restoring the insertion point at the time of the move.

[zero9178]

mlir/include/mlir/IR/Builders.h

297

Addressed in the final commit

mlir/include/mlir/IR/Builders.h

private:
     Block *block = nullptr;
     Block::iterator point;
   };
 
   /// RAII guard to reset the insertion point of the builder when destroyed.
   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;
   };
 
   /// Reset the insertion point to no location.  Creating an operation without a
   /// set insertion point is an error, but this can still be useful when the
   /// current insertion point a builder refers to is being removed.
   void clearInsertionPoint() {
     this->block = nullptr;