llvm / llvm/circt

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

Open
#3,235 2 comments 1 reaction 0 assignees View on GitHub

A pull request for this has already been merged.

  • #3243 by @cgyurgyik — merged
Calyx
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

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](https://github.com/llvm/circt/blob/main/test/Conversion/AffineToStaticLogic/loops.mlir) 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::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::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::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::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, llvm::function_ref, llvm::\
function_ref) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Rewrite/PatternApplicator.cpp:201:25
#17 0x0000000002fcdcf2 (anonymous namespace)::GreedyPatternRewriteDriver::simplify(llvm::MutableArrayRef) /home/mikeurbach/alloy/external/llvm-project/mlir/lib/Transforms/Utils/GreedyPatternRewrit\
eDriver.cpp:271:19
#18 0x0000000002fcd328 mlir::applyPatternsAndFoldGreedily(llvm::MutableArrayRef, 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) /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 >, bool, bool, bool, bool, llvm::function_ref, 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::function_ref, 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 >, 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](https://github.com/llvm/circt/blob/e00cf85163cd0acbfd44c32c8e6a154aa2f1a967/lib/Conversion/StaticLogicToCalyx/StaticLogicToCalyx.cpp#L1167) 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: https://github.com/llvm/circt/blob/8543511f83682a2025b9907806a1402c11a554d2/lib/Dialect/FIRRTL/Transforms/ExtractInstances.cpp#L143-L156.

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

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the failure with test/Conversion/AffineToStaticLogic/loops.mlir and the circt-opt command in the report. Read the shared pass logic in lib/Conversion/StaticLogicToCalyx/StaticLogicToCalyx.cpp, especially the state used near line 1167 and the module/function processing in runOnOperation. Done means the passes process multiple functions without stale state or the reported assertion.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.