llvm / clangir

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

[CIR][CIRGen] Improve switch support for unrecheable code #528

Open wenpen opened 5 months ago

wenpen commented 5 months ago

Support non-block case and statementw that don't belong to any case region, fix #520 #521

Lancern commented 5 months ago

Actually the body of a switch statement can be neither of case, default, or compound:

int f(int x) {
  switch (x)
    return 1;
  return 2;
}

This is accepted by clang (f returns 2 unconditionally) and since you're working on this I believe we can further resolve this in this PR.

wenpen commented 5 months ago

@Lancern Added CaseOpKind_NE and buildCaseNoneStmt() to handle the unreachable statements. Thanks for pointing out it!

bcardosolopes commented 5 months ago

Since you are already working on this and they are all related, this PR should also fix the other issues and include testscases from #521 and #522

Lancern commented 5 months ago

Well, things are a little tricky in the case when the body of a switch statement is not one of case, default, or compound. These statements are NOT simply unreachable, consider:

void foo(int x) {
  switch (x)
    while (condition()) {
  case 42:
      do_something();
    }
}

The while statement is indeed reachable when x is 42. In such a case switch behaves just like a goto.

wenpen commented 5 months ago

It becomes a little complex when we consider case across scope, the current definition of SwitchOp is not enough to express the semantics.

The definition assume the size of case attributes is same with regions, unfortunately the region may be nested. For example, there should be 2 case attribute with only 1 SwitchOp region (or said 2 nested region) in following code.

switch (x)
  case 9: {
    x++;
    case 7:
      x++;
  }

I'm not sure what a reasonable SwitchOp definition should be. A preliminary idea is using label and branch somehow, then the cir is like

cir.scope {
  cir.switch (...) [
    9: ^bb0
    7: ^bb1
  ] {
    ^bb0:
      cir.scope {
        ...
        ^bb1:
          ...
      }
  }
}

Otherwise I noticed goto didn't support branch across scope as well, maybe the problem is related? Do you have any suggestions?

Lancern commented 5 months ago

Since switch statements are more or less like a syntax sugar for goto, we may learn some ideas from the way goto is implemented in CIR: #508 .

bcardosolopes commented 5 months ago

It becomes a little complex when we consider case across scope

Oh right, you don't need to solve this problem for the moment, let's focus on the testcases that don't involve scope crossing. @gitoleg is working on https://github.com/llvm/clangir/pull/508, once that lands we can do incremental work to fix it.

bcardosolopes commented 5 months ago

Let me know when this comes out of Draft state and I'll take a look again

wenpen commented 5 months ago

Appreciate for the suggestions! This pr is ready for review now. @Lancern @bcardosolopes

wenpen commented 5 months ago

Comments addressed. Found some new corner-case, so rewrite some function and add a few UT.

bcardosolopes commented 4 months ago

Let me know once this is ready for review again!

wenpen commented 4 months ago

Hi, this pr is ready for review, thanks~

bcardosolopes commented 4 months ago

@wenpen still working on this? I don't usually look at draft PRs, just trying to make sure if there's something I should be looking here.

wenpen commented 4 months ago

@bcardosolopes Yes, just be a little busy recently, I will update the pr and request review form you later days, thanks~

bcardosolopes commented 3 months ago

I'm going to resume reviewing this, sorry for the delay!

bcardosolopes commented 3 months ago

I landed https://github.com/llvm/clangir/pull/611 which has some comments related to this PR (cc: @piggynl)