[Calyx] SCF and StaticLogic to Calyx passes should clear state.

[mikeurbach]

These passes are maintaining some complex state, but aren't doing anything to clean it up. This manifests as issues when you try to run them on a module with multiple functions. I just hit this by trying to run this test through the AffineToStaticLogic pass, and run the result through StaticLogicToCalyx:

circt-opt test/Conversion/AffineToStaticLogic/loops.mlir -convert-affine-to-staticlogic -lower-static-logic-to-calyx=top-level-function=dot

This asserts like so:

circt-opt: /home/mikeurbach/alloy/external/llvm-project/mlir/lib/IR/Types.cpp:92: unsigned int mlir::Type::getIntOrFloatBitWidth() const: Assertion `isIntOrFloat() && "only integers and floats have a bitwidth"'\
 failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: build/bin/circt-opt external/circt/test/Conversion/AffineToStaticLogic/loops.mlir -convert-affine-to-staticlogic -lower-static-logic-to-calyx=top-level-function=dot
 #0 0x000000000130f1ea llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/mikeurbach/alloy/external/llvm-project/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x000000000130f39b PrintStackTraceSignalHandler(void*) /home/mikeurbach/alloy/external/llvm-project/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x000000000130da36 llvm::sys::RunSignalHandlers() /home/mikeurbach/alloy/external/llvm-project/llvm/lib/Support/Signals.cpp:103:5
 #3 0x000000000130fac5 SignalHandler(int) /home/mikeurbach/alloy/external/llvm-project/llvm/lib/Support/Unix/Signals.inc:407:1
 #4 0x00007fe5195323c0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x143c0)
 #5 0x00007fe518dd203b raise /build/glibc-sMfBJT/glibc-2.31/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #6 0x00007fe518db1859 abort /build/glibc-sMfBJT/glibc-2.31/stdlib/abort.c:81:7
 #7 0x00007fe518db1729 get_sysdep_segment_value /build/glibc-sMfBJT/glibc-2.31/intl/loadmsgcat.c:509:8
 #8 0x00007fe518db1729 _nl_load_domain /build/glibc-sMfBJT/glibc-2.31/intl/loadmsgcat.c:970:34
 #9 0x00007fe518dc3006 (/lib/x86_64-linux-gnu/libc.so.6+0x34006)
#10 0x000000000230004c mlir::Type::getIntOrFloatBitWidth() const /home/mikeurbach/alloy/external/llvm-project/mlir/lib/IR/Types.cpp:0:3
#11 0x0000000002022834 circt::staticlogictocalyx::RewriteMemoryAccesses::partiallyLower(circt::calyx::AssignOp, mlir::PatternRewriter&) const /home/mikeurbach/alloy/external/circt/lib/Conversion/StaticLogicToCa\
lyx/StaticLogicToCalyx.cpp:1167:14
#12 0x0000000002022be0 circt::staticlogictocalyx::PartialLoweringPattern<circt::calyx::AssignOp, mlir::OpRewritePattern>::matchAndRewrite(circt::calyx::AssignOp, mlir::PatternRewriter&) const::'lambda'()::opera\
tor()() const /home/mikeurbach/alloy/external/circt/lib/Conversion/StaticLogicToCalyx/StaticLogicToCalyx.cpp:571:39
#13 0x0000000002022b8f void mlir::RewriterBase::updateRootInPlace<circt::staticlogictocalyx::PartialLoweringPattern<circt::calyx::AssignOp, mlir::OpRewritePattern>::matchAndRewrite(circt::calyx::AssignOp, mlir:\
:PatternRewriter&) const::'lambda'()>(mlir::Operation*, circt::calyx::AssignOp&&) /home/mikeurbach/alloy/external/llvm-project/llvm/../mlir/include/mlir/IR/PatternMatch.h:501:5
#14 0x0000000002022705 circt::staticlogictocalyx::PartialLoweringPattern<circt::calyx::AssignOp, mlir::OpRewritePattern>::matchAndRewrite(circt::calyx::AssignOp, mlir::PatternRewriter&) const /home/mikeurbach/a\
lloy/external/circt/lib/Conversion/StaticLogicToCalyx/StaticLogicToCalyx.cpp:570:14
#15 0x0000000001f5b2db mlir::detail::OpOrInterfaceRewritePatternBase<circt::calyx::AssignOp>::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&) const /home/mikeurbach/alloy/external/llvm-project/llvm/..\
/mlir/include/mlir/IR/PatternMatch.h:329:12
#16 0x0000000003034a5f mlir::PatternApplicator::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&, llvm::function_ref<bool (mlir::Pattern const&)>, llvm::function_ref<void (mlir::Pattern const&)>, llvm::\
function_ref<mlir::LogicalResult (mlir::Pattern const&)>) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Rewrite/PatternApplicator.cpp:201:25
#17 0x0000000002fcdcf2 (anonymous namespace)::GreedyPatternRewriteDriver::simplify(llvm::MutableArrayRef<mlir::Region>) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Transforms/Utils/GreedyPatternRewrit\
eDriver.cpp:271:19
#18 0x0000000002fcd328 mlir::applyPatternsAndFoldGreedily(llvm::MutableArrayRef<mlir::Region>, mlir::FrozenRewritePatternSet const&, mlir::GreedyRewriteConfig) /home/mikeurbach/alloy/external/llvm-project/mlir/\
lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp:406:8
#19 0x0000000001e29002 mlir::applyPatternsAndFoldGreedily(mlir::Operation*, mlir::FrozenRewritePatternSet const&, mlir::GreedyRewriteConfig) /home/mikeurbach/alloy/external/llvm-project/llvm/../mlir/include/mli\
r/Transforms/GreedyPatternRewriteDriver.h:71:10
#20 0x0000000001ff1d42 circt::staticlogictocalyx::StaticLogicToCalyxPass::runPartialPattern(mlir::RewritePatternSet&, bool) /home/mikeurbach/alloy/external/circt/lib/Conversion/StaticLogicToCalyx/StaticLogicToC\
alyx.cpp:2320:11
#21 0x0000000001fea78c circt::staticlogictocalyx::StaticLogicToCalyxPass::runOnOperation() /home/mikeurbach/alloy/external/circt/lib/Conversion/StaticLogicToCalyx/StaticLogicToCalyx.cpp:2418:39
#22 0x0000000003157fea mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Pass/Pass.cpp:471:21
#23 0x00000000031585e4 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::Pipelin\
eParentInfo const*) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Pass/Pass.cpp:534:16
#24 0x0000000003159eac mlir::PassManager::runPasses(mlir::Operation*, mlir::AnalysisManager) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Pass/Pass.cpp:837:10
#25 0x0000000003159dcc mlir::PassManager::run(mlir::Operation*) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Pass/Pass.cpp:817:60
#26 0x00000000026a0d0d performActions(llvm::raw_ostream&, bool, bool, llvm::SourceMgr&, mlir::MLIRContext*, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>) /home/mikeurbach/alloy/external/llvm-pro\
ject/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:79:17
#27 0x000000000269fc03 processBuffer(llvm::raw_ostream&, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer> >, bool, bool, bool, bool, llvm::function_ref<mlir::LogicalResult (mlir::Pass\
Manager&)>, mlir::DialectRegistry&, llvm::ThreadPool*) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:119:12
#28 0x000000000269f9c5 mlir::MlirOptMain(llvm::raw_ostream&, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer> >, llvm::function_ref<mlir::LogicalResult (mlir::PassManager&)>, mlir::Di\
alectRegistry&, bool, bool, bool, bool, bool) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:169:10
#29 0x000000000269fdca mlir::MlirOptMain(llvm::raw_ostream&, std::unique_ptr<llvm::MemoryBuffer, std::default_delete<llvm::MemoryBuffer> >, mlir::PassPipelineCLParser const&, mlir::DialectRegistry&, bool, bool,\
 bool, bool, bool) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:189:10
#30 0x00000000026a09b1 mlir::MlirOptMain(int, char**, llvm::StringRef, mlir::DialectRegistry&, bool) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp:293:14
#31 0x00000000012aa2ab main /home/mikeurbach/alloy/external/circt/tools/circt-opt/circt-opt.cpp:64:23
#32 0x00007fe518db30b3 __libc_start_main /build/glibc-sMfBJT/glibc-2.31/csu/../csu/libc-start.c:342:3
#33 0x00000000012aa11e _start (build/bin/circt-opt+0x12aa11e)
Aborted (core dumped)

If I pull out the dot function from that test file, and run it in isolation, the pass works no problem.

I ran the failure under a debugger, and found the dest that is having getIntOrFloatBitWidth called here had nothing to do with a memory, so the fact that state.isInputPortOfMemory returned true is quite concerning.

Since that method is just comparing Values (by pointer), and MLIR will reuse pointers as Values are destroyed and created, I believe the state is becoming stale, and needs to be cleaned up. I'm not exactly sure when in the pass this should happen, but as these are a ModuleOp passes, I'd expect them to be able to process an entire module with potentitally multiple functions, like in my test. We can probably clean up before starting to work on each function. Other passes that maintain complex state have similar clean ups, for example:

circt/lib/Dialect/FIRRTL/Transforms/ExtractInstances.cpp

Lines 143 to 156 in 8543511

	CircuitOp circuitOp = getOperation();
	anythingChanged = false;
	anyFailures = false;
	annotatedModules.clear();
	dutRootModules.clear();
	dutModules.clear();
	extractionWorklist.clear();
	extractionPaths.clear();
	originalInstanceParents.clear();
	extractedInstances.clear();
	instPrefices.clear();
	moduleNamespaces.clear();
	circuitNamespace.clear();
	circuitNamespace.add(circuitOp);

.

These passes both share the same guts, which are currently duplicated, so it might be best to finish that clean up before addressing this. I'm not in a hurry to get this fixed, I just noticed it and wanted to log it. It also wouldn't be the end of the world to apply the fix in two places for now.

cc @mortbopet @cgyurgyik

[cgyurgyik]

Makes sense. It would be nice if we could guarantee RAII for things like this.

[cgyurgyik]

I've followed a similar pattern for clearing state here, but the bug still exists.