[MLIR][NFC] Builder InsertionGuard::saveInsertionPoint -> savedInsertionPoint

[bondhugula]

Summary

Change misleading method name. This isn't saving an insertion point but
returning the saved insertion point.

[rriddle]

I'm not +1 on this change. The function is a verb, i.e. it is requesting that the builder "save" the current insertion point inside of an InsertionPoint and return it.

[rriddle]

Verbifying the function name would be better, i.e. getSavedInsertionPoint.

[rriddle]

But I still don't see a reason to change.

[rriddle]

This also matches the same verbiage in other parts of LLVM, IRBuilder::saveIP/llvm::SaveAndRestore/etc.

[bondhugula]

In D77935#1975886, @rriddle wrote:

I'm not +1 on this change. The function is a verb, i.e. it is requesting that the builder "save" the current insertion point inside of an InsertionPoint and return it.

Hmm... but it's not saving the current insertion point. It's only returning the saved one. It's the InsertionGuard ctor that's saving the insertion point. Am I missing something?

[bondhugula]

In D77935#1975891, @rriddle wrote:

This also matches the same verbiage in other parts of LLVM, IRBuilder::saveIP/llvm::SaveAndRestore/etc.

Oh I see - the saveInsertionPoint here is used to refer to the insertion point to use for saving?

[rriddle]

In D77935#1975923, @bondhugula wrote:

In D77935#1975891, @rriddle wrote:

This also matches the same verbiage in other parts of LLVM, IRBuilder::saveIP/llvm::SaveAndRestore/etc.

Oh I see - the saveInsertionPoint here is used to refer to the insertion point to use for saving?

Yeah, sorry about the short replies. The way that it is intended to be read is that you are saving the current insertion point of the builder and storing it somewhere else. That is why there is the companion restore method, so that you can restore a previously saved insertion point. The reason why "saved" sounds weird is that the builder isn't saving an insertion point, it just has one.

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), savedIp(builder.savedInsertionPoint()) {}
+    ~InsertionGuard() { builder.restoreInsertionPoint(savedIp); }
 
   private:
     OpBuilder &builder;
-    OpBuilder::InsertPoint ip;
+    OpBuilder::InsertPoint savedIp;
   };
 
   /// 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;
     insertPoint = Block::iterator();
   }
 
-  /// Return a saved insertion point.
-  InsertPoint saveInsertionPoint() const {
+  /// Returns the saved insertion point.
+  InsertPoint savedInsertionPoint() const {
     return InsertPoint(getInsertionBlock(), getInsertionPoint());
   }
 
   /// Restore the insert point to a previously saved point.
   void restoreInsertionPoint(InsertPoint ip) {
     if (ip.isSet())
       setInsertionPoint(ip.getBlock(), ip.getPoint());
     else

mlir/lib/EDSC/Builders.cpp

 }
 
 /// Sets the insertion point of the builder to 'newInsertPt' for the duration
 /// of the scope. The existing insertion point of the builder is restored on
 /// destruction.
 mlir::edsc::ScopedContext::ScopedContext(OpBuilder &builder,
                                          OpBuilder::InsertPoint newInsertPt,
                                          Location location)
-    : builder(builder), prevBuilderInsertPoint(builder.saveInsertionPoint()),
+    : builder(builder), prevBuilderInsertPoint(builder.savedInsertionPoint()),
       location(location),
       enclosingScopedContext(ScopedContext::getCurrentScopedContext()),
       nestedBuilder(nullptr) {
   getCurrentScopedContext() = this;
   builder.restoreInsertionPoint(newInsertPt);
 }
 
 mlir::edsc::ScopedContext::~ScopedContext() {

mlir/lib/Parser/Parser.cpp

ParseResult OperationParser::parseRegion(
     bool isIsolatedNameScope) {
   // Parse the '{'.
   if (parseToken(Token::l_brace, "expected '{' to begin a region"))
     return failure();
 
   // Check for an empty region.
   if (entryArguments.empty() && consumeIf(Token::r_brace))
     return success();
-  auto currentPt = opBuilder.saveInsertionPoint();
+  auto currentPt = opBuilder.savedInsertionPoint();
 
   // Push a new named value scope.
   pushSSANameScope(isIsolatedNameScope);
 
   // Parse the first block directly to allow for it to be unnamed.
   Block *block = new Block();
 
   // Add arguments to the entry block.