clangir icon indicating copy to clipboard operation
clangir copied to clipboard

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

Open wenpen opened this issue 11 months ago • 16 comments

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

wenpen avatar Apr 01 '24 13:04 wenpen

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.

Lancern avatar Apr 02 '24 03:04 Lancern

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

wenpen avatar Apr 02 '24 12:04 wenpen

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

bcardosolopes avatar Apr 03 '24 19:04 bcardosolopes

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.

Lancern avatar Apr 04 '24 16:04 Lancern

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?

wenpen avatar Apr 08 '24 08:04 wenpen

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 .

Lancern avatar Apr 08 '24 09:04 Lancern

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 avatar Apr 10 '24 00:04 bcardosolopes

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

bcardosolopes avatar Apr 10 '24 18:04 bcardosolopes

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

wenpen avatar Apr 12 '24 06:04 wenpen

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

wenpen avatar Apr 17 '24 10:04 wenpen

Let me know once this is ready for review again!

bcardosolopes avatar Apr 25 '24 22:04 bcardosolopes

Hi, this pr is ready for review, thanks~

wenpen avatar Apr 26 '24 07:04 wenpen

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

bcardosolopes avatar May 08 '24 22:05 bcardosolopes

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

wenpen avatar May 09 '24 08:05 wenpen

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

bcardosolopes avatar Jun 03 '24 22:06 bcardosolopes

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

bcardosolopes avatar Jun 06 '24 22:06 bcardosolopes