[CIR][Transforms] Move MergeCleanupsPass logic into Op::fold method

[Kritoooo]

This pr is a part of #593 . Move RemoveEmptyScope and RemoveEmptySwitch logic into Op::fold method and modify tests. Here are my questions during the development process:

1. Since the NumResults for BrOp (and several other operations) is 0, when performing legalizeWithFold, it always enters the following block: mlir/lib/Transforms/Utils/DialectConversion.cpp

   // An empty list of replacement values indicates that the fold was in-place.
   // As the operation changed, a new legalization needs to be attempted.
   if (replacementValues.empty())
    return legalize(op, rewriter);

   If this process occurs during -cir-flat-to-llvm or -cir-to-mlir, it will again enter: mlir/lib/Transforms/Utils/DialectConversion.cpp

   // If the operation isn't legal, try to fold it in-place.
   // TODO: Should we always try to do this, even if the op is
   // already legal?
   if (succeeded(legalizeWithFold(op, rewriter))) {
    LLVM_DEBUG({
      logSuccess(logger, "operation was folded");
      logger.startLine() << logLineComment;
    });
    return success();
   }

   This results in double folding, causing errors. My current solution is to perform a MergeCleanupsPass before entering -cir-flat-to-llvm and -cir-to-mlir to avoid the described situation.

2. After implementing cir.Scope::fold, issues arise in --debug mode for unknown reasons. The problem occurs at the following statements: mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp

   #ifndef NDEBUG
        Operation *dumpRootOp = getDumpRootOp(op);
   #endif // NDEBUG
        if (foldResults.empty()) {
          // Op was modified in-place.
          notifyOperationModified(op);
          changed = true;
          LLVM_DEBUG(logSuccessfulFolding(dumpRootOp));

3. In the ToMLIR path, should we perform the FlattenCFGPass? If we perform createFlattenCFGPass, the lowering of SCF will fail. However, if we do not perform it, it seems difficult to handle cir.goto.

[Kritoooo]

I also noticed that #482 has not been updated for a long time. If needed, I can implement the logic in Op::fold.

[bcardosolopes]

This results in double folding, causing errors. My current solution is to perform a MergeCleanupsPass before entering -cir-flat-to-llvm and -cir-to-mlir to avoid the described situation.

Nice catch! Why this process happens twice? Or why/what-circunstances double folding is getting trigger? Is flattening also calling into fold operations? Your solution seems fine, but I'm afraid we need to understand the problem to prevent this "double folding" to pop up whenever we introduce new passes.

2. After implementing cir.Scope::fold, issues arise in --debug mode for unknown reasons. The problem occurs at the following statements:

This needs not to happen, I can't tell from quick looking, sounds like you need debugger to the rescue.

3. In the ToMLIR path, should we perform the FlattenCFGPass? If we perform createFlattenCFGPass, the lowering of SCF will fail.

Right, if you lower out of structured control flow you loose a bunch of the MLIR nice stuff.

However, if we do not perform it, it seems difficult to handle cir.goto.

I'd work on testcases that make sense for you or your goal, instead of just trying to reach some type of lowering completeness out of CIR. I'd expect code full of goto's not to be the most interesting stuff someone would want to use MLIR for.

[bcardosolopes]

I also noticed that #482 has not been updated for a long time. If needed, I can implement the logic in Op::fold.

Feel free to work on #482, that hasn't had any update so time to make progress regardless

[bcardosolopes]

On another note, this PR is implementing both scope and switch, if only one of these operations are causing the said problems, I'd suggest you first land the part that matters, and the "harder parts" you add on the description can be left out to another PR.

[Kritoooo]

Nice catch! Why this process happens twice? Or why/what-circunstances double folding is getting trigger? Is flattening also calling into fold operations?

The double folding occurs during applyPartialConversion (ToLLVM or ToMLIR). In applyPartialConversion, there is an attempt to legalize an Op. https://github.com/llvm/clangir/blob/47df94c4c53d635b0c5ea9d132afaa533a5d3c7e/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp#L3565-L3566 https://github.com/llvm/clangir/blob/55e1496cd96ed23cfcf40c11fa2cb2945c484632/mlir/lib/Transforms/Utils/DialectConversion.cpp#L3609-L3615 In convertOperations: https://github.com/llvm/clangir/blob/55e1496cd96ed23cfcf40c11fa2cb2945c484632/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2624-L2626 In convert: https://github.com/llvm/clangir/blob/55e1496cd96ed23cfcf40c11fa2cb2945c484632/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2570-L2573 During the legalization, an isn't legal Op is legalized with legalizeWithFold. https://github.com/llvm/clangir/blob/47df94c4c53d635b0c5ea9d132afaa533a5d3c7e/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2030-L2040 In legalizeWithFold, operations with empty results undergo legalization again. https://github.com/llvm/clangir/blob/47df94c4c53d635b0c5ea9d132afaa533a5d3c7e/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2075-L2078 After returning to legalize, since it is still an isn't legal Op, it undergoes legalizeWithFold again. This seems to be a loop.
Therefore, if the result of the fold is empty, we need to complete the folding before the ToLLVM and ToMLIR passes, that is, before applyPartialConversion, otherwise, the above situation will occur.

This needs not to happen, I can't tell from quick looking, sounds like you need debugger to the rescue.

It only happens when running:

./build/Debug/bin/cir-opt -cir-to-mlir --debug ./clang/test/CIR/Lowering/ThroughMLIR/scope.cir

And from my tests, the error occurs when the scf dialect is loaded:

Load new dialect in Context scf
ImplicitTypeIDRegistry::lookupOrInsert(mlir::DestinationStyleOpInterface)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::ParallelCombiningOpInterface)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::detail::OpToOpPassAdaptor)
//===-------------------------------------------===//
Processing operation : 'cir.scope'(0x574b7d06fe20) {
ImplicitTypeIDRegistry::lookupOrInsert(mlir::OpTrait::HasRecursiveMemoryEffects<Empty>)
} -> success : operation was folded
//===-------------------------------------------===//
** Modified: 'cir.scope'(0x574b7d06fe20)
// *** IR Dump After Successful Folding ***
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:

However, when I remove the scf dialect and run:

./build/Debug/bin/cir-opt -cir-to-mlir --debug ./clang/test/CIR/Lowering/ThroughMLIR/scope.cir

the problem disappears:

ImplicitTypeIDRegistry::lookupOrInsert(mlir::bufferization::AllocationOpInterface)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::RuntimeVerifiableOpInterface)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::DestructurableTypeInterface)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::detail::OpToOpPassAdaptor)
//===-------------------------------------------===//
Processing operation : 'cir.scope'(0x57874bee12a0) {
ImplicitTypeIDRegistry::lookupOrInsert(mlir::OpTrait::HasRecursiveMemoryEffects<Empty>)
} -> success : operation was folded
//===-------------------------------------------===//
** Modified: 'cir.scope'(0x57874bee12a0)
// *** IR Dump After Successful Folding ***
cir.scope {
}

Through the debugger, I pinpointed the issue to occur during Op->dump(), which seems to be related to AsmPrinter. However, I am unsure what changes the loading of the scf dialect introduces that cause this issue. From my tests, running cir-opt --debug with ToLLVM and without the scf dialect on ToMLIR does not produce any issues. Of course, running without --debug also does not cause any problems.

Now, I have moved the logic of ScopeOp::fold back to MergeCleanups and added comments, hoping that someone will be able to fix it :)

[bcardosolopes]

Thanks for the full explanation, very nice! I wish I knew more about SCF to perhaps provide some insight on the root cause here =(

Now, I have moved the logic of ScopeOp::fold back to MergeCleanups and added comments, hoping that someone will be able to fix it :)

That works for me. One more promissing alternative though, is to enable MergeCleanups in voidConvertCIRToMLIRPass::runOnOperation() as a pre-requisite pass, which should get rid of the scope before you hit SCF and friends. wdyt?

Also, you need to update the PR because we just did a rebase against upstream!