Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bcardosolopes opened this issue May 8, 2024 · 8 comments
Open

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

bcardosolopes opened this issue May 8, 2024 · 8 comments
Labels
good first issue Good for newcomers

Comments

@bcardosolopes
Copy link
Member

@seven-mile uses this for casts in #591, we should do the same for the other ops if possible.

@orbiri-ns
Copy link

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

@bcardosolopes
Copy link
Member Author

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

@Kritoooo
Copy link
Contributor

Kritoooo commented Jun 3, 2024

@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
Copy link
Contributor

orbiri commented Jun 3, 2024

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
Copy link
Contributor

Kritoooo commented Jun 3, 2024

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
Copy link
Member Author

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

@Kritoooo
Copy link
Contributor

Kritoooo commented Jun 5, 2024

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants