[CIR][CIRGen][Bugfix] Fixes switch-case sub statements

[gitoleg]

This PR fixes CIR generation for the switch-case cases like the following:

case 'a':
default:
 ... 

or

default:
case 'a':
...

i.e. when the default clause is sub-statement of the case one and vice versa.

[sitio-couto]

@gitoleg nice catch!

Could you add a test in clang/test/CIR/CodeGen/switch.cpp validating your fix?

[gitoleg]

Sure!

[bcardosolopes]

Thanks for fixing this! Besides @sitio-couto points, can you please run clang-format while updating the PR?

[gitoleg]

@sitio-couto take a look again please: added tests + applied clang-format

[sitio-couto]

@gitoleg LGTM!

[gitoleg]

I got your point. Now, the question is - how it should look like?
Consider the example bellow:

  case 5:  
  default:
  case 4:
    break;
  }

The are several ideas I have now. First one is to collect these cases under CaseOpKind::Anyof and emit CIR somehow like the following :

case (anyof, [4, 5, default] ) {
 ....
}

In this case we need also fix printer (and probably parser ) in CIRDialect.cpp.

Another idea here is to insert jumps from the case (anyof, [4,5] ) to default region via JumpDest.

@bcardosolopes @sitio-couto What do you think? Any other ideas?

[sitio-couto]

First one is to collect these cases under CaseOpKind::Anyof

@gitoleg I like this idea: it's terse and retains all info from the source code statement. My only nitpick here would be to maintain the same order as the case/default statements appear (e.g. anyof, [5, default, 4] instead of anyof, [4, 5, default]). But, as you've mentioned, it is certainly more work.

If you prefer a quicker alternative, maybe we could just emit empty regions with a cir.yield fallthrough whenever we hit the default clause. It's not as elegant as your suggestion, but should be less work while retaining source code info for diagnostics.

So this:

  case 6:
  case 5:  
  default:
  case 4:
    break;

Would look like this:

  case (anyof, [6, 5]) {
    cir.yield fallthrough
  }
  case (default) {
    cir.yield fallthrough
  }
  case (equal, 4) {
    cir.yield break
  }

[bcardosolopes]

Good design points, thanks for bringing it up!

case (anyof, [4, 5, default] ) {
 ....
}

This looks nice, but I rather we start a bit more simple / closer to source code (for the sake of more accurate code analysis), and later as part of LoweringPrepare or some other optimization pass we could perhaps transform to this mode, or something similar.

In this case we need also fix printer (and probably parser ) in CIRDialect.cpp.

Yep, let's not worry about this right now though!

Another idea here is to insert jumps from the case (anyof, [4,5] ) to default region via JumpDest.

We should stay away as much as possible from goto's at this level, cause it complicates things like the lifetime checker. But it could be considered as a later pass, just as mentioned for the other approach above.

It's not as elegant as your suggestion, but should be less work while retaining source code info for diagnostics.

So this:

  case 6:
  case 5:  
  default:
  case 4:
    break;

Would look like this:

  case (anyof, [6, 5]) {
    cir.yield fallthrough
  }
  case (default) {
    cir.yield fallthrough
  }
  case (equal, 4) {
    cir.yield break
  }

My take is that we should start like this (no early optimizations), and any improvements can be made on top after CIRGen on behalf of extra transformation passes.

[gitoleg]

@bcardosolopes @sitio-couto
The PR updated. Now, the fallthrough are inserted!
Implementation
Now we don't create regions in the buildSwitchStmt method and do it during recursive calls of buildCaseStmt and buildDefaultStmt, since we need more regions than earlier because of nested sub expressions

[bcardosolopes]

This is awesome, thanks a bunch!