[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

Fri May 12 00:30:39 PDT 2023

junaire marked 10 inline comments as done.
junaire added inline comments.

================
Comment at: clang/include/clang/Interpreter/Value.h:160-162
+  // Interpreter, QualType are stored as void* to reduce dependencies.
+  void *Interp = nullptr;
+  void *OpaqueType = nullptr;
----------------
v.g.vassilev wrote:
> sgraenitz wrote:
> > v.g.vassilev wrote:
> > > aaron.ballman wrote:
> > > > junaire wrote:
> > > > > aaron.ballman wrote:
> > > > > > v.g.vassilev wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > v.g.vassilev wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Why don't forward declares suffice if we're storing the information by pointer?
> > > > > > > > > This is a performance-critical class. We literally measure the instruction count for it. We practically cannot include anything in this header file because the class needs to included as part of the interpreter runtime. For example:
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > #include <clang/Interpreter/Value.h>
> > > > > > > > > Value ResultV;
> > > > > > > > > gInterpreter->evaluate("float i = 12.3; i++", &V);
> > > > > > > > > printf("Value is %d\n", ResultV.castAs<int>());
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > This is how you can do things in Cling. This not yet there but that's our next step.
> > > > > > > > > 
> > > > > > > > > For performance reasons we have the `ValueKind` optimization which allows us to perform most of the operations we need very fast. There are some operations such as printing the concrete type which need the actual `QualType` and so on but they are outside of the performance critical paths and it is okay to resort back to the real types providing the level of accuracy we need.
> > > > > > > > > 
> > > > > > > > That sounds like it's going to lead to maintenance problems in the long term, right? I can't think of another header file we say "don't touch this because it may impact runtime performance", and so I can easily imagine someone breaking your expectation that this file can't include anything else.
> > > > > > > > 
> > > > > > > > Is there a long-term plan for addressing this?
> > > > > > > We have a few components like the Lexer that are extremely prone to performance regressions.
> > > > > > > 
> > > > > > > In terms for a longer-term plan in addressing this there are some steps could help IMO. First, this component is relatively standalone and very few changes will be required over time, for these I am hoping to be listed as a reviewer. Second, we can add a comment in the include area, making a note that including anything here will degrade the performance of almost all interpreted code. Third, we will find out about this in our downstream use-cases as the things get significantly slower.
> > > > > > > 
> > > > > > > Neither is silver bullet but that's probably the best we could do at that time. Btw, we might be able to add a test that's part of LLVM's performance analysis infrastructure.
> > > > > > > Neither is silver bullet but that's probably the best we could do at that time. Btw, we might be able to add a test that's part of LLVM's performance analysis infrastructure.
> > > > > > 
> > > > > > Yeah, we should probably consider doing that. But to make sure I understand the performance concerns... when we change functionality in the lexer, we (potentially) slow down the lexing phase of compilation. That's straightforward and unsurprising. But in this case, it sounds like the act of including another header file in this header file will cause a runtime performance concern, even if no other changes are made. If I'm correct, I can't think of anything else in the compiler that works like that.
> > > > > I believe what @v.g.vassilev means is that the repl itself might include `Value.h` as a part of *runtime*, so if the header is heavy, you can notice the repl is slowed down. (That's obvious) So keep in mind we're breaking the boundary between the compiled code and interpreted code (It's kinda confusing) here it actually impacts interpreted code.
> > > > > I believe what @v.g.vassilev means is that the repl itself might include Value.h as a part of *runtime*, so if the header is heavy, you can notice the repl is slowed down. (That's obvious) So keep in mind we're breaking the boundary between the compiled code and interpreted code (It's kinda confusing) here it actually impacts interpreted code.
> > > > 
> > > > I'm not certain that's a reasonable design choice to make. Or, stated somewhat differently, I'm really uncomfortable with having header files we can't maintain because changing them impacts runtime performance in surprising ways. That's not a sustainable design even if we think this header will be reasonably stable. We need *some* amount of abstraction here so that we can have a clean design for the REPL interpreter without NFC code changes impacting performance. Otherwise, people will be discouraged from adding comments to this file (those take time to lex, after all), or using long identifiers (longer identifiers take longer to lex than shorter ones), or including what is used instead of using `void *` (as being discussed here), and so on.
> > > > 
> > > > This is quite probably something you've already thought about plenty, but... could we add an abstraction layer so that the interpreter side of things has a "low-token-count" interface that dispatches through to the actual implementation?
> > > > > I believe what @v.g.vassilev means is that the repl itself might include Value.h as a part of *runtime*, so if the header is heavy, you can notice the repl is slowed down. (That's obvious) So keep in mind we're breaking the boundary between the compiled code and interpreted code (It's kinda confusing) here it actually impacts interpreted code.
> > > > 
> > > > I'm not certain that's a reasonable design choice to make. Or, stated somewhat differently, I'm really uncomfortable with having header files we can't maintain because changing them impacts runtime performance in surprising ways. That's not a sustainable design even if we think this header will be reasonably stable. We need *some* amount of abstraction here so that we can have a clean design for the REPL interpreter without NFC code changes impacting performance. Otherwise, people will be discouraged from adding comments to this file (those take time to lex, after all), or using long identifiers (longer identifiers take longer to lex than shorter ones), or including what is used instead of using `void *` (as being discussed here), and so on.
> > > 
> > > All valid points. I guess we have seen some changes related to compilation speed in the past in the STLExtras.h (iirc, although I cannot find the right commit). We did particular changes to the header file to reduce the compilation time of some large TU builds. I'd think that's more like the case of `stddef.h` and similar headers in the resource directory. The more we add the worse becomes the compiler startup time.
> > > 
> > > > 
> > > > This is quite probably something you've already thought about plenty, but... could we add an abstraction layer so that the interpreter side of things has a "low-token-count" interface that dispatches through to the actual implementation?
> > > 
> > > Yes, I have a plan that's quite ambitious (and a draft RFC): basically the idea is any `#include` to become a no-op for the compiler unless something is actually needed.
> > > 
> > > I understand your concern here but I don't really know how to address it in this particular patch.
> > > 
> > > 
> > >> [...] include Value.h as a part of *runtime*, so if the header is heavy, you can notice the repl is slowed down. (That's obvious) So keep in mind we're breaking the boundary between the compiled code and interpreted code (It's kinda confusing) here it actually impacts interpreted code.
> > > I'm really uncomfortable with having header files we can't maintain because changing them impacts runtime performance in surprising ways. [...] could we add an abstraction layer so that the interpreter side of things has a "low-token-count" interface that dispatches through to the actual implementation?
> > 
> > Agree. What about splitting this up in 3 parts?
> > (1) Private API: interface as consumed by the libclangInterpreter
> > (2) Public API: interface as consumed by tools, other LLVM projects and out-of-tree
> > (3) Client API: the actual runtime-only parts with low-token-count etc.
> > 
> > (1) and (2) would be here in the source-tree. (3) is a resource file similar to intrinsics headers, I believe they have very similar requirements on performance, maintenance and versioning.
> I agree with this suggestion. We have actually tried `(3)` and discovered that it is challenging to write unit tests for it. The main issue is that the clang tests have been designed to avoid relying on the resource directory, which is relative to the clang binary. Moreover, some builds redefine the location of the resource directory during compilation, making it difficult to locate and pass to a unit test whose location is independent of the clang binary's location...
Hi @aaron.ballman, I would like to try to explain why we don't have a better approach again.

`Value` is very special, it's not a regular runtime that would only be used by the REPL, instead, it acts like a messager, and connects the compiled code and the interpreted code. That said both sides need to reference it. so it can't be put into the resource directory, or the host compiler that is used for compiling clang-repl can't find it, then we fail to compile. What's more, it can't be put into compiler-rt too, same reason, `Interpreter.h` needs to include the header. The only possible location I can think of is the regular `include/clang/Interpter`.

When it comes to maintenance issues, I believe that's fine. IIUC your main concern is that people will be afraid to touch this file since it has a surprising performance impact. However, the whole header is only about the declaration of `Value` class and it should only be used for this purpose. So unless someone is working on clang-repl, it's unlikely for them to touch it. Another thing I would like to mention is that the performance is only noticeably dropped if we pulled some large headers, like `<string>`, `<memory>`, or something like that. NFC changes like fixing typos, adding comments, and adjusting function names are totally fine. (They can't simply refactor the `void*` thing, or the whole program simplify fail to compile)

We could also add some comments to indicate the specificity of this header, and why people should be cautious when touching it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215