[PATCH] D135764: [clang][Interp] Implement for loops

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked an inline comment as done.
tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeStmtGen.cpp:361
+  this->emitLabel(IncLabel);
+  if (Inc && !this->discard(Inc))
+    return false;
----------------
aaron.ballman wrote:
> What does the call to `discard()` do? Evaluate the expression and discard the results? (If so, the API might want for a better name at some point, as it can also read like "do nothing with this expression".)
Yes, it's the same problem that https://reviews.llvm.org/D136013 tries to solve. If `Inc` evaluates to a value, it will leave that on the stack and //something// needs to remove it from there, which is what `discard()` does.


================
Comment at: clang/test/AST/Interp/loops.cpp:216-226
+  constexpr int f4() {
+    int i = 0;
+    for (;;) {
+      i = i + 1;
+
+      if (i == 5)
+        break;
----------------
aaron.ballman wrote:
> I'd love to see a stress test for the compile time performance of:
> ```
> int func() {
>   int i = 0;
>   for (;;) {
>     if (i == __INT_MAX__)
>       break;
>     ++i;
>   }
>   return i;
> }
> static_assert(func() == __INT_MAX__);
> ```
> I don't think there's value to testing specifically how long it takes for this to execute, but we should probably start thinking about how to validate performance characteristics pretty soon. Perhaps relative performance compared to the existing constant expression evaluator would be interesting though?
> Perhaps relative performance compared to the existing constant expression evaluator would be interesting though?

Definitely. My plan was basically to get some base functionality going and then always keep an eye on performance. Your test doesn't work because the current interpreter has a step limit, but for a simple loop up until `100'000`, the current interpreter takes 42s here and the new one takes 2s.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135764/new/

https://reviews.llvm.org/D135764