[PATCH] D131888: [Clang][Interp] Add some basic functionality to the experimental new constant interpreter

Mon Aug 15 06:52:04 PDT 2022

tbaeder added a comment.

Thanks for the fast reply, Erich!

In D131888#3723017 <https://reviews.llvm.org/D131888#3723017>, @erichkeane wrote:

> So you're right, we haven't seen much work on this interpreter in a while, and I have no real experience with reviewing it at all, which I'm sure is the experience of most of my co-reviewers.

Sure, that's expected.

> At the same time, I believe there is good potential in this interpreter, so I would love to see progress on it.
>
> SO, if you could start by splitting this patch up into the smallest parts possible, it would be really appreciated.  This patch seems to be doing a lot, and I'm a bit overwhelmed trying to review it all at once.

I actually have several smaller patches locally but merged them into one for review :) I'll try to find a middle way.

> Additionally, I suspect there is going to be a LOT of tests, so I'd suggest writing 1 whole 'test file' per topic (instead of throwing everything into interp.cpp).  I also suspect that there is need for more in depth testing than that.  For exmaple, 'if' statements likely need to check things like side-effects and init statements/etc.
>
> One valuable strategy might be to temporarily switch this over to the 'default' interpreter in your local workspace, make changes, and see if you can add a new 'run' line to any existing tests that each individual change you make causes to pass.

I tried running the clang testsuite locally with the new interpreter, but basically all tests fail because something is missing, so I think that's gonna have to wait. But adding more files and tests on a per-patch basis is a good approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131888