SCF IndexSwitch to nested If-Else

[jiahanxie353]

This transform is meant for dialects that inherently does not have a switch control statement (like Calyx)

[jiahanxie353]

#7669 supports nested if-else control

also referencing https://github.com/calyxir/calyx/issues/1177 and https://github.com/calyxir/calyx/issues/1423 if they are relevant

[rachitnigam]

This needs test programs before we can merge it. Please add some tests and tag me when that's done.

[jiahanxie353]

Bumping this again.

Thanks for the reminder, I'll change and push again soon!

[jiahanxie353]

I think I should move to directory lib/Transforms/SwitchToIf.cpp since it's transforming an scf operation to another, not lowering to another dialect?

Also tagging code owners for their opinions @mikeurbach @mortbopet , thanks!

[rachitnigam]

Yeah, I think that sounds like a good idea!

[rachitnigam]

Let's add tests before making @mortbopet or @mikeurbach review this PR...

[jiahanxie353]

bumping this thank you for reviewing folks

[jiahanxie353]

Thanks, refined the description and code based on your review!

[jiahanxie353]

hi @rachitnigam , does this patch look good to you now?

[seldridge]

This patch failed nightly valgrind regressions: https://github.com/llvm/circt/actions/runs/12030131054/job/33536896034#step:6:5229

RUN: at line 1: /__w/circt/circt/build/bin/circt-opt -split-input-file --switch-to-if /__w/circt/circt/test/Transforms/switch-to-if.mlir | /__w/circt/circt/build/bin/FileCheck /__w/circt/circt/test/Transforms/switch-to-if.mlir
+ /__w/circt/circt/build/bin/circt-opt -split-input-file --switch-to-if /__w/circt/circt/test/Transforms/switch-to-if.mlir
+ /__w/circt/circt/build/bin/FileCheck /__w/circt/circt/test/Transforms/switch-to-if.mlir
==31750== Invalid read of size 8
==31750==    at 0x2BF56A0: mlir::Block::getParentOp() (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36ACFD8: llvm::LogicalResult llvm::function_ref<llvm::LogicalResult (mlir::Pattern const&)>::callback_fn<(anonymous namespace)::OperationLegalizer::legalizeWithPattern(mlir::Operation*, mlir::ConversionPatternRewriter&)::$_2>(long, mlir::Pattern const&) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E77C0: 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<llvm::LogicalResult (mlir::Pattern const&)>)::$_0>(long) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E462E: 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&)>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A276C: (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A1806: mlir::OperationConverter::convert(mlir::ConversionPatternRewriter&, mlir::Operation*) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A29AE: mlir::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36AA2BA: mlir::applyPartialConversion(mlir::Operation*, mlir::ConversionTarget const&, mlir::FrozenRewritePatternSet const&, mlir::ConversionConfig) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2BBA2C4: (anonymous namespace)::IndexSwitchToIfPass::runOnOperation() (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x3840D05: mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x3841531: mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x3843D5D: mlir::PassManager::run(mlir::Operation*) (in /__w/circt/circt/build/bin/circt-opt)
==31750==  Address 0x69074a8 is 24 bytes inside a block of size 72 free'd
==31750==    at 0x483CFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==31750==    by 0x2BBAF7E: SwitchToIfConversion::matchAndRewrite(mlir::scf::IndexSwitchOp, mlir::scf::IndexSwitchOpAdaptor, mlir::ConversionPatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2BBAB5F: mlir::OpConversionPattern<mlir::scf::IndexSwitchOp>::matchAndRewrite(mlir::Operation*, llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A1770: mlir::ConversionPattern::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E79E9: 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<llvm::LogicalResult (mlir::Pattern const&)>)::$_0>(long) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E462E: 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&)>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A276C: (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A1806: mlir::OperationConverter::convert(mlir::ConversionPatternRewriter&, mlir::Operation*) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A29AE: mlir::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36AA2BA: mlir::applyPartialConversion(mlir::Operation*, mlir::ConversionTarget const&, mlir::FrozenRewritePatternSet const&, mlir::ConversionConfig) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2BBA2C4: (anonymous namespace)::IndexSwitchToIfPass::runOnOperation() (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x3840D05: mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) (in /__w/circt/circt/build/bin/circt-opt)
==31750==  Block was alloc'd at
==31750==    at 0x483BE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==31750==    by 0x2BF96E3: mlir::OpBuilder::createBlock(mlir::Region*, llvm::ilist_iterator<llvm::ilist_detail::node_options<mlir::Block, true, false, void, false, void>, false, false>, mlir::TypeRange, llvm::ArrayRef<mlir::Location>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x310E77A: mlir::scf::IfOp::build(mlir::OpBuilder&, mlir::OperationState&, mlir::TypeRange, mlir::Value, bool) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2A6D60A: mlir::scf::IfOp mlir::OpBuilder::create<mlir::scf::IfOp, mlir::ValueTypeRange<mlir::ResultRange>, mlir::Value&, bool>(mlir::Location, mlir::ValueTypeRange<mlir::ResultRange>&&, mlir::Value&, bool&&) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2BBAEE1: SwitchToIfConversion::matchAndRewrite(mlir::scf::IndexSwitchOp, mlir::scf::IndexSwitchOpAdaptor, mlir::ConversionPatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x2BBAB5F: mlir::OpConversionPattern<mlir::scf::IndexSwitchOp>::matchAndRewrite(mlir::Operation*, llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A1770: mlir::ConversionPattern::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&) const (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E79E9: 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<llvm::LogicalResult (mlir::Pattern const&)>)::$_0>(long) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36E462E: 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&)>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A276C: (anonymous namespace)::OperationLegalizer::legalize(mlir::Operation*, mlir::ConversionPatternRewriter&) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A1806: mlir::OperationConverter::convert(mlir::ConversionPatternRewriter&, mlir::Operation*) (in /__w/circt/circt/build/bin/circt-opt)
==31750==    by 0x36A29AE: mlir::OperationConverter::convertOperations(llvm::ArrayRef<mlir::Operation*>) (in /__w/circt/circt/build/bin/circt-opt)
==31750==

[seldridge]

Blamelessly reverted in https://github.com/llvm/circt/commit/b987492739855b3d508922bbebb84e12f2649e8f to keep nightly clean.

[jiahanxie353]

https://github.com/llvm/circt/actions/runs/12030131054/job/33536896034#step:6:5229

Thanks for catching it @seldridge . Let me try to fix it.

Do you know how to run valgrind locally so that I can reproduce the error? I looked at the integration test output and it has:

FAILED: tools/circt/test/CMakeFiles/check-circt /__w/circt/circt/build/tools/circt/test/CMakeFiles/check-circt 
cd /__w/circt/circt/build/tools/circt/test && /usr/bin/python3.9 /__w/circt/circt/build/./bin/llvm-lit -v --show-unsupported --vg /__w/circt/circt/build/tools/circt/test

but I can't find all those directories locally. For example, there's no circt/build/tools/circt/test executable

[seldridge]

It may just work if you take that test and run in through valgrind, e.g., valgrind circt-opt .... However, to reproduce, I added some options to cmake that were used to turn on valgrind for all tests:

-DLLVM_LIT_ARGS="-v --show-unsupported --vg"

That then runs every test under valgrind. There might be a way of hacking this onto an existing build or invoking lit manually that I am unaware of.

[jiahanxie353]

It may just work if you take that test and run in through valgrind, e.g., valgrind circt-opt .... However, to reproduce, I added some options to cmake that were used to turn on valgrind for all tests:

-DLLVM_LIT_ARGS="-v --show-unsupported --vg"

That then runs every test under valgrind. There might be a way of hacking this onto an existing build or invoking lit manually that I am unaware of.

I see, thank you!