llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
312 stars 86 forks source link

Move more logic from MergeCleanups into Op::fold methods #593

Open bcardosolopes opened 2 months ago

bcardosolopes commented 2 months ago

@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.

orbiri-ns commented 2 months ago

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

bcardosolopes commented 2 months ago

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

Kritoooo commented 1 month ago

@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.

orbiri commented 1 month ago

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 :)

Kritoooo commented 1 month ago

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.:)

bcardosolopes commented 1 month ago

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

Kritoooo commented 1 month ago

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.

bcardosolopes commented 1 month ago

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