[CFToHandshake][DialectConversion] When rollback is removed, PartialLowerRegion becomes illegal.

[mikeurbach]

See the discussion here: https://discourse.llvm.org/t/rfc-a-new-one-shot-dialect-conversion-driver/79083/46

I followed the steps to set the flag in MLIR to disallow rollback, and CFToHandshake fails.

/scratch/verif/mikeu/circt/build/bin/circt-opt -lower-cf-to-handshake /scratch/verif/mikeu/circt/test/Conversion/CFToHandshake/invalid.mlir -split-input-file -verify-diagnostics # RUN: at line 1
+
LLVM ERROR: pattern 'PartialLowerRegion' rollback of IR modifications requested
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.
Stack dump:
0.      Program arguments: /scratch/verif/mikeu/circt/build/bin/circt-opt -lower-cf-to-handshake /scratch/verif/mikeu/circt/test/Conversion/CFToHandshake/invalid.mlir -split-input-file -verify-diagnostics
 #0 0x0000000001efe17d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /scratch/verif/mikeu/circt/llvm/llvm/lib/Support/Unix/Signals.inc:804:11
 #1 0x0000000001efe6fb PrintStackTraceSignalHandler(void*) /scratch/verif/mikeu/circt/llvm/llvm/lib/Support/Unix/Signals.inc:888:1
 #2 0x0000000001efc696 llvm::sys::RunSignalHandlers() /scratch/verif/mikeu/circt/llvm/llvm/lib/Support/Signals.cpp:105:5
 #3 0x0000000001efef3d SignalHandler(int, siginfo_t*, void*) /scratch/verif/mikeu/circt/llvm/llvm/lib/Support/Unix/Signals.inc:418:7
 #4 0x00007f6efe168cf0 __restore_rt (/lib64/libpthread.so.0+0x12cf0)
 #5 0x00007f6efcb66acf raise (/lib64/libc.so.6+0x4eacf)
 #6 0x00007f6efcb39ea5 abort (/lib64/libc.so.6+0x21ea5)
 #7 0x0000000001e6a6e4 llvm::report_fatal_error(llvm::Twine const&, bool) /scratch/verif/mikeu/circt/llvm/llvm/lib/Support/ErrorHandling.cpp:126:5
 #8 0x00000000063a35e5 mlir::detail::ConversionPatternRewriterImpl::undoRewrites(unsigned int, llvm::StringRef) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:1233:5
 #9 0x00000000063b6241 mlir::detail::ConversionPatternRewriterImpl::resetState((anonymous namespace)::RewriterState, llvm::StringRef) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:0:3
#10 0x00000000063b61cb (anonymous namespace)::OperationLegalizer::legalizeWithPattern(mlir::Operation*, mlir::ConversionPatternRewriter&)::$_21::operator()(mlir::Pattern const&) const /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:2174:18
#11 0x00000000063b5ecd void llvm::function_ref<void (mlir::Pattern const&)>::callback_fn<(anonymous namespace)::OperationLegalizer::legalizeWithPattern(mlir::Operation*, mlir::ConversionPatternRewriter&)::$_21>(long, mlir::Pattern const&) /scratch/verif/mikeu/circt/llvm/\
llvm/include/llvm/ADT/STLFunctionalExtras.h:46:5
#12 0x000000000644a769 llvm::function_ref<void (mlir::Pattern const&)>::operator()(mlir::Pattern const&) const /scratch/verif/mikeu/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:69:5
#13 0x0000000006449577 mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<llvm::LogicalResult (mlir::Pattern const&)>)::$_\
1::operator()() const /scratch/verif/mikeu/circt/llvm/mlir/lib/Rewrite/PatternApplicator.cpp:231:9
#14 0x00000000064491b5 void llvm::function_ref<void ()>::callback_fn<mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<ll\
vm::LogicalResult (mlir::Pattern const&)>)::$_1>(long) /scratch/verif/mikeu/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:46:5
#15 0x0000000001e3f569 llvm::function_ref<void ()>::operator()() const /scratch/verif/mikeu/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:69:5
#16 0x000000000644ac7d void mlir::MLIRContext::executeAction<mlir::ApplyPatternAction, mlir::Pattern const&>(llvm::function_ref<void ()>, llvm::ArrayRef<mlir::IRUnit>, mlir::Pattern const&) /scratch/verif/mikeu/circt/llvm/mlir/include/mlir/IR/MLIRContext.h:281:3
#17 0x0000000006447b2c mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::function_ref<llvm::LogicalResult (mlir::Pattern const&)>) /sc\
ratch/verif/mikeu/circt/llvm/mlir/lib/Rewrite/PatternApplicator.cpp:233:9
#18 0x00000000063b57d4 (anonymous namespace)::OperationLegalizer::legalizeWithPattern(mlir::Operation*, mlir::ConversionPatternRewriter&) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:2197:21
#19 0x00000000063a9b10 (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:2086:17
#20 0x00000000063a9483 mlir::OperationConverter::convert(mlir::ConversionPatternRewriter&, mlir::Operation*) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:2596:26
#21 0x00000000063a9e2f mlir::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:2696:16
#22 0x00000000063adb29 mlir::applyPartialConversion(llvm::ArrayRef<mlir::Operation*>, mlir::ConversionTarget const&, mlir::FrozenRewritePatternSet const&, mlir::ConversionConfig) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:3388:22
#23 0x00000000063adc2d mlir::applyPartialConversion(mlir::Operation*, mlir::ConversionTarget const&, mlir::FrozenRewritePatternSet const&, mlir::ConversionConfig) /scratch/verif/mikeu/circt/llvm/mlir/lib/Transforms/Utils/DialectConversion.cpp:3394:10
#24 0x00000000047360f9 circt::handshake::partiallyLowerRegion(llvm::function_ref<llvm::LogicalResult (mlir::Region&, mlir::ConversionPatternRewriter&)> const&, mlir::MLIRContext*, mlir::Region&) /scratch/verif/mikeu/circt/lib/Conversion/CFToHandshake/CFToHandshake.cpp:20\
6:7
#25 0x0000000003592a49 llvm::LogicalResult circt::handshake::runPartialLowering<circt::handshake::HandshakeLowering, llvm::MapVector<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Operation*>>, llvm::DenseMap<mlir::Value, unsigned int, llvm::DenseMapInfo\
<mlir::Value, void>, llvm::detail::DenseMapPair<mlir::Value, unsigned int>>, llvm::SmallVector<std::pair<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Operation*>>>, 0u>>, bool, llvm::MapVector<mlir::Value, std::vector<mlir::Operation*, std::allocator<m\
lir::Operation*>>, llvm::DenseMap<mlir::Value, unsigned int, llvm::DenseMapInfo<mlir::Value, void>, llvm::detail::DenseMapPair<mlir::Value, unsigned int>>, llvm::SmallVector<std::pair<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Operation*>>>, 0u>>, bo\
ol>(circt::handshake::HandshakeLowering&, llvm::LogicalResult (circt::handshake::HandshakeLowering::*)(mlir::ConversionPatternRewriter&, llvm::MapVector<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Operation*>>, llvm::DenseMap<mlir::Value, unsigned int\
, llvm::DenseMapInfo<mlir::Value, void>, llvm::detail::DenseMapPair<mlir::Value, unsigned int>>, llvm::SmallVector<std::pair<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Operation*>>>, 0u>>, bool), llvm::MapVector<mlir::Value, std::vector<mlir::Operati\
on*, std::allocator<mlir::Operation*>>, llvm::DenseMap<mlir::Value, unsigned int, llvm::DenseMapInfo<mlir::Value, void>, llvm::detail::DenseMapPair<mlir::Value, unsigned int>>, llvm::SmallVector<std::pair<mlir::Value, std::vector<mlir::Operation*, std::allocator<mlir::Op\
eration*>>>, 0u>>&, bool&) /scratch/verif/mikeu/circt/include/circt/Conversion/CFToHandshake.h:175:10
#26 0x0000000004759b43 llvm::LogicalResult circt::handshake::lowerRegion<mlir::func::ReturnOp, circt::handshake::ReturnOp>(circt::handshake::HandshakeLowering&, bool, bool, mlir::Value) /scratch/verif/mikeu/circt/include/circt/Conversion/CFToHandshake.h:230:14
#27 0x0000000004740377 lowerFuncOp(mlir::func::FuncOp, mlir::MLIRContext*, bool, bool) /scratch/verif/mikeu/circt/lib/Conversion/CFToHandshake/CFToHandshake.cpp:1732:16
#28 0x000000000473fc84 (anonymous namespace)::CFToHandshakePass::runOnOperation() /scratch/verif/mikeu/circt/lib/Conversion/CFToHandshake/CFToHandshake.cpp:1757:18
#29 0x00000000066fabeb mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int)::$_7::operator()() const /scratch/verif/mikeu/circt/llvm/mlir/lib/Pass/Pass.cpp:0:17
#30 0x00000000066fab85 void llvm::function_ref<void ()>::callback_fn<mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int)::$_7>(long) /scratch/verif/mikeu/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:46:\
5
#31 0x0000000001e3f569 llvm::function_ref<void ()>::operator()() const /scratch/verif/mikeu/circt/llvm/llvm/include/llvm/ADT/STLFunctionalExtras.h:69:5
#32 0x00000000066feb9d void mlir::MLIRContext::executeAction<mlir::PassExecutionAction, mlir::Pass&>(llvm::function_ref<void ()>, llvm::ArrayRef<mlir::IRUnit>, mlir::Pass&) /scratch/verif/mikeu/circt/llvm/mlir/include/mlir/IR/MLIRContext.h:281:3
#33 0x00000000066f62aa mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) /scratch/verif/mikeu/circt/llvm/mlir/lib/Pass/Pass.cpp:534:17
#34 0x00000000066f6864 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) /scratch/verif/mikeu/circt/llvm/mlir/lib/\
Pass/Pass.cpp:594:16
#35 0x00000000066f834c mlir::PassManager::runPasses(mlir::Operation*, mlir::AnalysisManager) /scratch/verif/mikeu/circt/llvm/mlir/lib/Pass/Pass.cpp:907:10
#36 0x00000000066f826f mlir::PassManager::run(mlir::Operation*) /scratch/verif/mikeu/circt/llvm/mlir/lib/Pass/Pass.cpp:887:60
...

@mortbopet do we need to maintain this PartialLowerRegion? At the very least, I don't think it needs to be a DialectConversion, since it just does a modifyOpInPlace and calls some function.

Seems like specifically the issue is maximizeSSA, which is making some IR modifications before potentially returning failure. If we really care about this being a DialectConversion, I think it could be made to delay modifications until we are sure the pattern can succeed, but that would require splitting addArgToTerminator and building an in-memory IRMapping or other table to keep track of rewrites until we can actually apply them all at the end.