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

Sun May 7 13:50:37 PDT 2023

v.g.vassilev 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;
----------------
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.

================
Comment at: clang/lib/Interpreter/InterpreterUtils.cpp:19
+  const llvm::APInt Addr(8 * sizeof(void *), Ptr);
+  return IntegerLiteral::Create(C, Addr, C.getUIntPtrType(), SourceLocation());
+}
----------------
aaron.ballman wrote:
> junaire wrote:
> > aaron.ballman wrote:
> > > This question applies more generally than just this function, but should we be requiring these interfaces to supply a `SourceLocation` rather than hard-coding no location information? That can make it easier to see what's going on when dumping the AST for debugging purposes, etc even if it's not necessary for diagnostics or other reasons. (I don't insist, just a speculative question about the interface.)
> > Keep in mind these helpers are actually used for *synthesizing* AST nodes, so there are no SourceLocations available here. If you dumped the AST, you probably will see something like:
> > ```
> > CallExpr 0x621000093270 'void'
> > |-ImplicitCastExpr 0x621000093250 'void (*)(void *, void *, void *, unsigned long long)' <FunctionToPointerDecay>
> > | `-DeclRefExpr 0x6210000931f8 'void (void *, void *, void *, unsigned long long)' lvalue Function 0x62100008fd98 '__clang_Interpreter_SetValueNoAlloc' 'void (void *, void *, void *, unsigned long long)'
> > |-CStyleCastExpr 0x621000093060 'void *' <IntegralToPointer>
> > | `-IntegerLiteral 0x621000093020 'unsigned long' 106515188942880
> > |-CStyleCastExpr 0x6210000930d0 'void *' <IntegralToPointer>
> > | `-IntegerLiteral 0x621000093090 'unsigned long' 106515188942912
> > |-CStyleCastExpr 0x621000093140 'void *' <IntegralToPointer>
> > | `-IntegerLiteral 0x621000093100 'unsigned long' 107820859101728
> > `-CStyleCastExpr 0x6210000931c8 'unsigned long long' <NoOp>
> >   `-ImplicitCastExpr 0x6210000931a8 'unsigned long long' <IntegralCast> part_of_explicit_cast
> >     `-ImplicitCastExpr 0x621000093188 'int' <LValueToRValue> part_of_explicit_cast
> >       `-DeclRefExpr 0x621000092ec0 'int' lvalue Var 0x621000092d50 'x' 'int'
> > ```
> > 
> > Horrible, but that's all we can do :( 
> Ah, that's true, this is all synthesized.
Outside of this review, we've been entertaining the idea of providing a source location scratch area which represents some memory region of the decompiled to text synthesized AST. That can solve a number of general issues with synthesized code but we have not got around to experiment with it.

================
Comment at: clang/lib/Interpreter/Value.cpp:91
+  static constexpr unsigned char Canary[8] = {0x4c, 0x37, 0xad, 0x8f,
+                                              0x2d, 0x23, 0x95, 0x91};
+};
----------------
aaron.ballman wrote:
> junaire wrote:
> > sgraenitz wrote:
> > > Can we add a comment with an explanation of the magic numbers please?
> > I'm uncertain if these numbers actually have specific meanings, I just copied them from Cling directly.
> That's all the more reason to document the numbers -- should they stay in sync with Cling?
I believe the explanation is at the use in `isAlive`:
```
  // Check whether the storage is valid by validating the canary bits.
  // If someone accidentally write some invalid bits in the storage, the canary
  // will be changed first, and `IsAlive` will return false then.
```

I'd suggest here we can say that these are random numbers that allow us to detect if someone wrote bits in our storage accidentally. Or actually refer the the documentation in `isAlive`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215