From 57cbeaa8b5de01cf72e739df467fb47f3867e1af Mon Sep 17 00:00:00 2001 From: Alex Zinenko Date: Wed, 20 May 2020 16:00:37 +0200 Subject: [PATCH] [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 --- mlir/examples/toy/Ch5/mlir/LowerToAffineLoops.cpp | 3 ++- mlir/examples/toy/Ch6/mlir/LowerToAffineLoops.cpp | 3 ++- mlir/examples/toy/Ch6/mlir/LowerToLLVM.cpp | 3 ++- mlir/examples/toy/Ch7/mlir/LowerToAffineLoops.cpp | 3 ++- mlir/examples/toy/Ch7/mlir/LowerToLLVM.cpp | 3 ++- mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp | 6 +++--- mlir/lib/Conversion/GPUToSPIRV/ConvertGPUToSPIRV.cpp | 2 +- 7 files changed, 14 insertions(+), 9 deletions(-) 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 operands, SmallVector loopIvs; for (auto dim : tensorType.getShape()) { auto loop = rewriter.create(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 operands, SmallVector loopIvs; for (auto dim : tensorType.getShape()) { auto loop = rewriter.create(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 @@ public: auto step = rewriter.create(loc, 1); auto loop = rewriter.create(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 operands, SmallVector loopIvs; for (auto dim : tensorType.getShape()) { auto loop = rewriter.create(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 @@ public: auto step = rewriter.create(loc, 1); auto loop = rewriter.create(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 @@ public: Value upperBound = lowerAffineUpperBound(op, rewriter); Value step = rewriter.create(loc, op.getStep()); auto f = rewriter.create(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 @@ public: bool hasElseRegion = !op.elseRegion().empty(); auto ifOp = rewriter.create(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(); }