Page MenuHomePhabricator

[clang][Interp] Implement switch statements
ClosedPublic

Authored by tbaeder on Nov 4 2022, 5:31 AM.

Details

Summary

This creates a temporary local variable for the condition variable, then emits all the comparisons for the case statements, followed by the sub statements of the case statements.

Diff Detail

Event Timeline

tbaeder created this revision.Nov 4 2022, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 5:31 AM
tbaeder requested review of this revision.Nov 4 2022, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 5:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 473212.Nov 4 2022, 6:16 AM
aaron.ballman added inline comments.Nov 28 2022, 2:49 PM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
421

How well does this handle large switch statements? Generated code sometimes winds up with thousands of cases, so I'm wondering if we need to have a jump table implementation for numerous cases and use the simple comparison implementation when there's only a small number of labels (and we can test to see what constitutes a good value for "small number of labels").

Also, one thing to be aware of (that hopefully won't matter TOO much in this case): the switch case list does not have a stable iteration order IIRC.

clang/test/AST/Interp/switch.cpp
17–19

lol, you should probably fix this so it's not so confusing to casual readers.

47

I think it would be good to add a non-trivial init and show that it fails when appropriate. e.g.,

struct Weirdo {
  consteval Weirdo(int);
  Weirdo(double);

  int Val = 1;
};

constexpr int whatever() {
  switch (Weirdo W(12); W.Val) {
  case 1: return 12;
  default: return 100;
  }
}

constexpr int whatever_else() {
  switch (Weirdo W(1.2); W.Val) { // Should get an error because of the init not being constexpr
  case 1: return 12;
  default: return 100;
  }
}

static_assert(whatever() == 12, "");
static_assert(whatever_else() == 12, ""); // Shouldn't compile because the function isn't constexpr
69

I'd like a test where the case value is itself a constant expression that is either valid or invalid:

constexpr int good() { return 1; }
constexpr int test(int val) {
  switch (val) {
  case good(): return 100;
  default: return -1;
  }
  return 0;
}
static_assert(test(1) == 100, "");

constexpr int bad(int val) { return val / 0; }
constexpr int another_test(int val) {
  switch (val) {
  case bad(val): return 100; // Should not compile
  default: return -1;
  }
  return 0;
}
static_assert(another_test(1) == 100, ""); // Should not compile
tbaeder added inline comments.Nov 29 2022, 12:45 AM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
421

With 10'000 case labels, it seems to be slightly slower if all case labels need to be iterated through and slightly faster if the first iteration already hits. I'm not too concerned about the performance here tbh.

clang/test/AST/Interp/switch.cpp
17–19

What exactly is the confusing part? :D That the case labels are out of order?

aaron.ballman added inline comments.Nov 29 2022, 4:54 AM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
421

Okay, we can address performance concerns later, if they arise. Thanks!

clang/test/AST/Interp/switch.cpp
17–19

The function is called isEven and 11, 13, 15 all return true due to fallthrough.

tbaeder updated this revision to Diff 478895.Nov 30 2022, 4:25 AM
tbaeder marked 2 inline comments as done.
tbaeder marked an inline comment as done.
tbaeder added inline comments.
clang/test/AST/Interp/switch.cpp
47

This is unfortunately hard do test with the new constant interpreter right now. It never emits the "never produces a constant expression" diagnostic because I removed the Run() call from isPotentialConstantExpression(). I need to revert that and instead fix the interpreter to correctly takes this mode into account.

And for the non-constexpr constructor, we will reject the call in the static_assert(), but not diagnose anything useful because we're not checking the constructor for constexpr-ness (this is part of https://reviews.llvm.org/D137563 I believe).

aaron.ballman accepted this revision.Nov 30 2022, 7:26 AM
aaron.ballman added inline comments.
clang/test/AST/Interp/switch.cpp
47

I'd be fine if the tests were added with FIXME comments (or commented out if they're too disruptive).

This revision is now accepted and ready to land.Nov 30 2022, 7:26 AM
shafik added inline comments.Dec 21 2022, 6:28 PM
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
403

The condition could be a class type w/ a conversion operator, this does not seem like it will handle this case. Maybe b/c you don't handle conversion operators yet but might be worth a FIXME or TODO.

This revision was landed with ongoing or failed builds.Wed, Jan 25, 4:21 AM
This revision was automatically updated to reflect the committed changes.