clangir icon indicating copy to clipboard operation
clangir copied to clipboard

Move more logic from MergeCleanups into Op::fold methods

Open bcardosolopes opened this issue 9 months ago • 8 comments

@seven-mile uses this for casts in https://github.com/llvm/clangir/pull/591, we should do the same for the other ops if possible.

bcardosolopes avatar May 08 '24 18:05 bcardosolopes

I may want to tackle this, but I’ll be available to get to it only later this month 🙏🏻🏝️

orbiri-ns avatar May 10 '24 10:05 orbiri-ns

@orbiri-ns np, I leave this to you (unless someone grabs it first, perhaps unlikely tho)

bcardosolopes avatar May 10 '24 17:05 bcardosolopes

@orbiri-ns Hello, how is your progress going? If you are too busy, I can do this. Of course, if you have already started, then just ignore my message.

Kritoooo avatar Jun 03 '24 06:06 Kritoooo

Hey, I have unfortunately not found time to tackle this and can give it away. I will note that I did start doing some research unto how do similar canonicalizations look like in scf dialect, and found that they heavily rely on the trivially-unused removal. I would suggest to go a bit deeper there before blindly implementing code that should be handled by core MLIR for us.

My experiments included running mlir-opt with debug and canonicalization flags on, and ran it on various scf.ifs, scf.execution_region and similar. I would be happy to see your research results here on how can we have similar behavior for CIR! (Even if the results indicate that we can't use MLIR core to help us). I promise to have shorter turnaround time :)

orbiri avatar Jun 03 '24 06:06 orbiri

Thank you for your suggestion. I will research this issue after completing some work related to ThroughMLIR. However, I cannot guarantee that I will be able to solve this issue because I am a newcomer. If there are any updates on this issue, I will post them here promptly. If anyone else is interested in this issue, please feel free to discuss it here.:)

Kritoooo avatar Jun 03 '24 06:06 Kritoooo

@Kritoooo thanks for following up with this! My workflow suggestion:

  • Do one operation at a time.
  • Start with the easier ones.
  • Start with ones we already handle in MergeCleanups.
  • Add new ones.

bcardosolopes avatar Jun 03 '24 21:06 bcardosolopes

Thank you for your suggestion. I have started moving RemoveRedundantBranches to BrOp::fold. So far, it is going well. However, I have encountered an issue: many original test files do not seem to apply the RemoveRedundantBranches optimization, causing the tests to fail. Do I need to modify all these test files to make them pass? Is there a better way to handle this? such as CIR/Lowering/loop.cir origin test

  cir.func @forWithBreakTerminatedScopeInBody(%arg0 : !cir.bool) {
      cir.for : cond {
        cir.condition(%arg0)
      } body {
        cir.scope { // FIXME(cir): Redundant scope emitted during C codegen.
          cir.break
        }
        cir.yield
      } step {
        cir.yield
      }
    cir.return
  }
// CHECK:  cir.func @forWithBreakTerminatedScopeInBody(%arg0: !cir.bool) {
// CHECK:    cir.br ^bb[[#COND:]]
// CHECK:  ^bb[[#COND]]:
// CHECK:    cir.brcond %arg0 ^bb[[#BODY:]], ^bb[[#EXIT:]]
// CHECK:  ^bb[[#BODY]]:
// CHECK:    cir.br ^bb[[#EX_SCOPE_IN:]]
// CHECK:  ^bb[[#EX_SCOPE_IN]]:
// CHECK:    cir.br ^bb[[#EXIT:]]
// CHECK:  ^bb[[#EX_SCOPE_EXIT:]]:
// CHECK:    cir.br ^bb[[#STEP:]]
// CHECK:  ^bb[[#STEP]]:
// CHECK:    cir.br ^bb[[#COND:]]
// CHECK:  ^bb[[#EXIT]]:
// CHECK:    cir.return
// CHECK:  }

move RemoveRedundantBranches to BrOp::fold

  llvm.func @forWithBreakTerminatedScopeInBody(%arg0: i8) attributes {cir.extra_attrs = #fn_attr} {
    llvm.br ^bb1
  ^bb1:  // 2 preds: ^bb0, ^bb3
    %0 = llvm.trunc %arg0 : i8 to i1
    llvm.cond_br %0, ^bb2, ^bb4
  ^bb2:  // pred: ^bb1
    llvm.br ^bb4
  ^bb3:  // no predecessors
    llvm.br ^bb1
  ^bb4:  // 2 preds: ^bb1, ^bb2
    llvm.return
  }

Sure, after moving the RemoveRedundantBranches logic to BrOp::fold, it can directly pass the merge-cleanups.cir test.

Kritoooo avatar Jun 05 '24 04:06 Kritoooo

You probably need to add -cir-merge-cleanups to cir-opt invocation?

bcardosolopes avatar Jun 06 '24 22:06 bcardosolopes