[Mlir-commits] [mlir] 57cbeaa - [mlir] Erase or clear blocks through ConversionPatternRewriter when applicable

[Alex Zinenko]

Author: Alex Zinenko
Date: 2020-05-20T16:12:05+02:00
New Revision: 57cbeaa8b5de01cf72e739df467fb47f3867e1af

URL: https://github.com/llvm/llvm-project/commit/57cbeaa8b5de01cf72e739df467fb47f3867e1af
DIFF: https://github.com/llvm/llvm-project/commit/57cbeaa8b5de01cf72e739df467fb47f3867e1af.diff

LOG: [mlir] Erase or clear blocks through ConversionPatternRewriter when applicable

Multiple places in the code base were erasing Blocks or operations in them
using in-place modifications (`Block::erase` or `Block::clear`) unknown to
ConversionPatternRewriter. These operations could not be undone if the pattern
failed and could lead to inconsistent in-memory state of the IR with dangling
pointers. Use `ConversionPatternRewriter::eraseOp` and `::eraseBlock` instead.

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

Added: 
    

Modified: 
    mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp
    mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp
    mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp
    mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp
    mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp
    mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
    mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp b/mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp
index 0988f5fe0c41..0514d8f6624b 100644
--- a/mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp
+++ b/mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp
@@ -72,7 +72,8 @@ static void lowerOpToLoops(Operation *op, ArrayRef<Value> operands,
   SmallVector<Value, 4> loopIvs;
   for (auto dim : tensorType.getShape()) {
     auto loop = rewriter.create<AffineForOp>(loc, /*lb=*/0, dim, /*step=*/1);
-    loop.getBody()->clear();
+    for (Operation &nested : *loop.getBody())
+      rewriter.eraseOp(&nested);
     loopIvs.push_back(loop.getInductionVar());
 
     // Terminate the loop body and update the rewriter insertion point to the

diff  --git a/mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp b/mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp
index 56629d7ae217..e7dceea44226 100644
--- a/mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp
+++ b/mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp
@@ -72,7 +72,8 @@ static void lowerOpToLoops(Operation *op, ArrayRef<Value> operands,
   SmallVector<Value, 4> loopIvs;
   for (auto dim : tensorType.getShape()) {
     auto loop = rewriter.create<AffineForOp>(loc, /*lb=*/0, dim, /*step=*/1);
-    loop.getBody()->clear();
+    for (Operation &nested : *loop.getBody())
+      rewriter.eraseOp(&nested);
     loopIvs.push_back(loop.getInductionVar());
 
     // Terminate the loop body and update the rewriter insertion point to the

diff  --git a/mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp b/mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp
index c6c49ec7db6e..2d169d80d6dc 100644
--- a/mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp
+++ b/mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp
@@ -69,7 +69,8 @@ class PrintOpLowering : public ConversionPattern {
       auto step = rewriter.create<ConstantIndexOp>(loc, 1);
       auto loop =
           rewriter.create<scf::ForOp>(loc, lowerBound, upperBound, step);
-      loop.getBody()->clear();
+      for (Operation &nested : *loop.getBody())
+        rewriter.eraseOp(&nested);
       loopIvs.push_back(loop.getInductionVar());
 
       // Terminate the loop body.

diff  --git a/mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp b/mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp
index 0988f5fe0c41..0514d8f6624b 100644
--- a/mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp
+++ b/mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp
@@ -72,7 +72,8 @@ static void lowerOpToLoops(Operation *op, ArrayRef<Value> operands,
   SmallVector<Value, 4> loopIvs;
   for (auto dim : tensorType.getShape()) {
     auto loop = rewriter.create<AffineForOp>(loc, /*lb=*/0, dim, /*step=*/1);
-    loop.getBody()->clear();
+    for (Operation &nested : *loop.getBody())
+      rewriter.eraseOp(&nested);
     loopIvs.push_back(loop.getInductionVar());
 
     // Terminate the loop body and update the rewriter insertion point to the

diff  --git a/mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp b/mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp
index c6c49ec7db6e..2d169d80d6dc 100644
--- a/mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp
+++ b/mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp
@@ -69,7 +69,8 @@ class PrintOpLowering : public ConversionPattern {
       auto step = rewriter.create<ConstantIndexOp>(loc, 1);
       auto loop =
           rewriter.create<scf::ForOp>(loc, lowerBound, upperBound, step);
-      loop.getBody()->clear();
+      for (Operation &nested : *loop.getBody())
+        rewriter.eraseOp(&nested);
       loopIvs.push_back(loop.getInductionVar());
 
       // Terminate the loop body.

diff  --git a/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp b/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
index 96b62969c8cd..b2012b7322b1 100644
--- a/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
+++ b/mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
@@ -350,7 +350,7 @@ class AffineForLowering : public OpRewritePattern<AffineForOp> {
     Value upperBound = lowerAffineUpperBound(op, rewriter);
     Value step = rewriter.create<ConstantIndexOp>(loc, op.getStep());
     auto f = rewriter.create<scf::ForOp>(loc, lowerBound, upperBound, step);
-    f.region().getBlocks().clear();
+    rewriter.eraseBlock(f.getBody());
     rewriter.inlineRegionBefore(op.region(), f.region(), f.region().end());
     rewriter.eraseOp(op);
     return success();
@@ -396,10 +396,10 @@ class AffineIfLowering : public OpRewritePattern<AffineIfOp> {
     bool hasElseRegion = !op.elseRegion().empty();
     auto ifOp = rewriter.create<scf::IfOp>(loc, cond, hasElseRegion);
     rewriter.inlineRegionBefore(op.thenRegion(), &ifOp.thenRegion().back());
-    ifOp.thenRegion().back().erase();
+    rewriter.eraseBlock(&ifOp.thenRegion().back());
     if (hasElseRegion) {
       rewriter.inlineRegionBefore(op.elseRegion(), &ifOp.elseRegion().back());
-      ifOp.elseRegion().back().erase();
+      rewriter.eraseBlock(&ifOp.elseRegion().back());
     }
 
     // Ok, we're done!

diff  --git a/mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp b/mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
index 15bd2d923c16..0f53487de4c5 100644
--- a/mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
+++ b/mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp
@@ -419,7 +419,7 @@ LogicalResult GPUModuleConversion::matchAndRewrite(
   // The spv.module build method adds a block with a terminator. Remove that
   // block. The terminator of the module op in the remaining block will be
   // legalized later.
-  spvModuleRegion.back().erase();
+  rewriter.eraseBlock(&spvModuleRegion.back());
   rewriter.eraseOp(moduleOp);
   return success();
 }