[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added subscribers: samitolvanen, pcc. v.g.vassilev added a comment. In D96033#2948243 , @leonardchan wrote: >> @leonardchan, @phosek, I am not aware of such flag. Do you know how much >> memory does LTO + clang-repl consume and would it make

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

leonardchan added a comment. > @leonardchan, @phosek, I am not aware of such flag. Do you know how much > memory does LTO + clang-repl consume and would it make sense to ping some of > the LTO folks for their advice? I played around a bit with adding a flag and it turned out to be not as

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2944625 , @phosek wrote: > In D96033#298 , @leonardchan > wrote: > >> We're still hitting the OOMs when building clang-repl with LTO even with >>

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

phosek added a comment. In D96033#298 , @leonardchan wrote: > We're still hitting the OOMs when building clang-repl with LTO even with > `-DLLVM_PARALLEL_LINK_JOBS=32`. While we don't build this target explicitly > in our toolchain, it is built when

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

leonardchan added a comment. We're still hitting the OOMs when building clang-repl with LTO even with `-DLLVM_PARALLEL_LINK_JOBS=32`. While we don't build this target explicitly in our toolchain, it is built when running tests via `stage2-check-clang`. Is there perhaps a simple cmake flag that

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. (Tabbed on the Submit button instead of finishing that quote block...) So I assume the stage2- targets are just invoking some ninja invocations in sequence? Anyway, what I think it would be helpful to see what link jobs were in progress. But I guess even with 32

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. In D96033#2768940 , @phosek wrote: > In D96033#2767884 , @teemperor wrote: > >> In D96033#2766502 , @phosek wrote: >> >>> In D96033#2766372

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

phosek added a comment. In D96033#2767884 , @teemperor wrote: > In D96033#2766502 , @phosek wrote: > >> In D96033#2766372 , @v.g.vassilev >> wrote: >> >>> In D96033#2766332

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @uweigand, thanks again for confirming the patch works for you. The differential revision is here https://reviews.llvm.org/D102756 @teemperor and @Hahnfeld, thanks for the diagnosis. I forgot to mention that we had some exchange on Discord and @phosek kindly

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

Hahnfeld added a comment. In D96033#2767884 , @teemperor wrote: > In D96033#2766502 , @phosek wrote: > >> In D96033#2766372 , @v.g.vassilev >> wrote: >> >>> In

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

uweigand added a comment. Yes, this patch fixes the problem for me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits mailing list

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. In D96033#2766502 , @phosek wrote: > In D96033#2766372 , @v.g.vassilev > wrote: > >> In D96033#2766332 , @phosek wrote: >> >>> We've started

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2766141 , @uweigand wrote: > In D96033#2765954 , @v.g.vassilev > wrote: > >> @hubert.reinterpretcast, thanks for the feedback. I have created a patch as >> discussed --

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

phosek added a comment. In D96033#2766372 , @v.g.vassilev wrote: > In D96033#2766332 , @phosek wrote: > >> We've started seeing `LLVM ERROR: out of memory` on our 2-stage LTO Linux >> builders after this change

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2766332 , @phosek wrote: > We've started seeing `LLVM ERROR: out of memory` on our 2-stage LTO Linux > builders after this change landed. It looks like linking `clang-repl` always > fails on our bot, but I've also

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

phosek added a comment. We've started seeing `LLVM ERROR: out of memory` on our 2-stage LTO Linux builders after this change landed. It looks like linking `clang-repl` always fails on our bot, but I've also seen OOM when linking `ClangCodeGenTests` and `FrontendTests`. Do you have any idea why

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

uweigand added a comment. In D96033#2765954 , @v.g.vassilev wrote: > @hubert.reinterpretcast, thanks for the feedback. I have created a patch as > discussed -- https://reviews.llvm.org/D102688 > > @uweigand, thanks for reaching out. I believe the patch

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @hubert.reinterpretcast, thanks for the feedback. I have created a patch as discussed -- https://reviews.llvm.org/D102688 @uweigand, thanks for reaching out. I believe the patch above should fix your setup. Could you confirm? Repository: rG LLVM Github

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

uweigand added a comment. Looks like this is also failing on s390x: error: Added modules have incompatible data layouts: E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64 (module) vs E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64 (jit) The problem here is that on s390x we use a

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2764946 , @hubert.reinterpretcast wrote: >> If that works on your platform I will happily open a review for the changes. > > From the behaviour observed in the 64-bit build, the 32-bit case might not >

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2762182 , @v.g.vassilev wrote: > That patch should probably get us to a point where we can mark the test as > `XFAIL: system-aix` I've applied that patch to my 64-bit LLVM build and it does cause the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2762134 , @hubert.reinterpretcast wrote: > In D96033#2762056 , @v.g.vassilev > wrote: > >> IIUC, AIX has a default `fno-integrated-as` and I am still puzzled (and >>

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2762056 , @v.g.vassilev wrote: > IIUC, AIX has a default `fno-integrated-as` and I am still puzzled (and > cannot find the relevant code) what is the program action kind for AIX in > that case

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2761581 , @hubert.reinterpretcast wrote: > In D96033#2761428 , > @hubert.reinterpretcast wrote: > >> If you have some ideas, please let me know. Meanwhile, I am trying out

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2761428 , @hubert.reinterpretcast wrote: > If you have some ideas, please let me know. Meanwhile, I am trying out > `system-aix` as the "feature" to XFAIL on. https://reviews.llvm.org/D102560 posted to

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. Once I add `-Xcc -fintegrated-as`, we get: $ cat /home/hstong/.Liodine/llvmproj/clang/test/Interpreter/execute.cpp | /home/hstong/.Nrtphome/.Liodine/llcrossbld/dev/build/bin/clang-repl -Xcc -fintegrated-as fatal error: error in backend: 64-bit XCOFF

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. @v.g.vassilev, thanks for working with me on this. I understand it is difficult to handle issues that appear on platforms and build configurations one does not have set up. I've added `-Xcc -v` and the output is below. It seems it has to do with the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2760067 , @hubert.reinterpretcast wrote: > In D96033#2759808 , @v.g.vassilev > wrote: > >> Hi @hubert.reinterpretcast, >> >> Would you mind testing this patch: > > Does the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2759808 , @v.g.vassilev wrote: > Hi @hubert.reinterpretcast, > > Would you mind testing this patch: Does the test try to generate native object files in some way? There is functionality (with some

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2759808 , @v.g.vassilev wrote: > Hi @hubert.reinterpretcast, > > Would you mind testing this patch: Running it now. I've applied the first diff here to the base of my previously reported result to

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. Hi @hubert.reinterpretcast, Would you mind testing this patch: diff diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp index 8de38c0afcd9..79acb5bd6898 100644 --- a/clang/lib/Interpreter/Interpreter.cpp +++

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2758189 , @lhames wrote: > Ok, looks like the JIT is getting the layout right, but clang-repl is > constructing a module with an little-endian layout for some reason. `clang-repl` is generating a module

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. In D96033#2757930 , @hubert.reinterpretcast wrote: > ... > Command Output (stderr): > > triple: powerpc64-ibm-aix7.2.0.0 > datalayout: E-m:a-i64:64-n32:64-S128-v256:256:256-v512:512:512 > error: Added modules have incompatible

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2757342 , @lhames wrote: > Hi Hubert, > > Could you apply the following patch and let me know the output from the > failing test? I'm trying to work out whether the JIT is getting the triple or > the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2757342 , @lhames wrote: > Hi Hubert, > > Could you apply the following patch and let me know the output from the > failing test? I'm trying to work out whether the JIT is getting the triple or > the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. In D96033#2757222 , @hubert.reinterpretcast wrote: > ... > I have a local build I can apply a patch to. Hi Hubert, Could you apply the following patch and let me know the output from the failing test? I'm trying to work out

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. In D96033#2757179 , @v.g.vassilev wrote: > In D96033#2757107 , @v.g.vassilev > wrote: > >> In D96033#2757077 , >>

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2757107 , @v.g.vassilev wrote: > In D96033#2757077 , > @hubert.reinterpretcast wrote: > >> @v.g.vassilev, the test does not appear to be appropriately set up for >> builds

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2757077 , @hubert.reinterpretcast wrote: > @v.g.vassilev, the test does not appear to be appropriately set up for builds > that default to a non-native target: >

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

hubert.reinterpretcast added a comment. @v.g.vassilev, the test does not appear to be appropriately set up for builds that default to a non-native target: https://lab.llvm.org/staging/#/builders/126/builds/371/steps/5/logs/FAIL__Clang__execute_cpp Can you fix or revert until there is a fix?

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. Thanks everybody for reviewing and help making this patch better!! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG44a4000181e1: [clang-repl] Land initial infrastructure for incremental parsing (authored by v.g.vassilev). Herald added a project: clang. Changed

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 344941. v.g.vassilev added a comment. clang-tidy and clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files: clang/include/clang/CodeGen/CodeGenAction.h

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rsmith accepted this revision. rsmith added a comment. Generally-speaking, we have a plan that I'm happy for us to work towards, and I'm happy for our progress towards that plan to be incremental. Even though this might not be fully in that direction right now, I think that's OK. CHANGES

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rsmith added inline comments. Comment at: clang/lib/Interpreter/IncrementalParser.cpp:226-235 + if (PP.getLangOpts().DelayedTemplateParsing) { +// Microsoft-specific: +// Late parsed templates can leave unswallowed "macro"-like tokens. +// They will seriously

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. Thanks, @teemperor. I have addressed your last comments, too. I wanted to use the opportunity to ping @rjmccall and @rsmith. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 342980. v.g.vassilev marked an inline comment as done. v.g.vassilev added a comment. - Check if we can use death tests; - Do not install clang-repl as being an early stage development. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. (Obviously this should still wait on Richard & John as I think they still have unaddressed objections) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor accepted this revision. teemperor added a comment. This revision is now accepted and ready to land. I believe everything I pointed out so far has been resolved. I still have one more nit about the unit test though (just to not make it fail on some Windows setups). FWIW, given that

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev marked 2 inline comments as done. v.g.vassilev added a comment. In D96033#2717749 , @teemperor wrote: > Sorry for the delay. I think all my points have been resolved beside the > insertion SourceLoc. Apologies, I overlooked this comment.

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 342485. v.g.vassilev added a comment. Address source location comment of teemperor. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files: clang/include/clang/CodeGen/CodeGenAction.h

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @rsmith ping2... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. Sorry for the delay. I think all my points have been resolved beside the insertion SourceLoc. Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184 + SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID()) +

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @rsmith ping... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @teemperor, could you take another look at this patch -- I believe most of your concerns are addressed now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2695748 , @rjmccall wrote: > In D96033#2695622 , @v.g.vassilev > wrote: > >> Would it make sense to have each `Decl` point to its owning PTU, similarly >> to what we do for

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2695622 , @v.g.vassilev wrote: > Would it make sense to have each `Decl` point to its owning PTU, similarly to > what we do for the owning module (`Decl::getOwningModule`)? I think that's the interface we want, but

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. Thank you for the details -- will be really useful to me and that architecture will define away several hard problems we tried to solve over the years. In D96033#2693910 , @rjmccall wrote: > Sorry for the delay. That seems

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. Sorry for the delay. That seems like a reasonable summary of our discussion. Let me try to lay out the right architecture for this as I see it. Conceptually, we can split the translation unit into a sequence of partial translation units (PTUs). Every declaration

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @rjmccall, @rsmith, ping... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. >> We probably need to talk about it. > > +1. Do you use discord/slack/skype? I will try to summarize the discussion here. @rjmccall , @rsmith please feel free to correct me if I am wrong or add important points that I missed. The discussion focused on supporting

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. In D96033#2632848 , @rjmccall wrote: > In D96033#2630978 , @v.g.vassilev > wrote: > I'm nervous in general about the looming idea of declaration unloading, but the fact

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. To be clear, what I'm trying to say is that — absent consensus that it's a major architecture shift is appropriate — we need to consider what functionality is reasonably achievable without it. I'm sure that covers most of what you're trying to do; it just may not

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2630978 , @v.g.vassilev wrote: >>> I'm nervous in general about the looming idea of declaration unloading, but >>> the fact that it's been working in Cling for a long time gives me some >>> confidence that we can do

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. >> I'm nervous in general about the looming idea of declaration unloading, but >> the fact that it's been working in Cling for a long time gives me some >> confidence that we can do that in a way that's not prohibitively expensive >> and invasive. > > I don't

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2630585 , @rsmith wrote: > @rjmccall I'd like your input on this patch in particular, if you have time. I've been specifically avoiding paying any attention to this patch because it sounds like an enormous time-sink

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rsmith added a comment. @rjmccall I'd like your input on this patch in particular, if you have time. I'm nervous in general about the looming idea of declaration unloading, but the fact that it's been working in Cling for a long time gives me some confidence that we can do that in a way that's

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 330428. v.g.vassilev added a comment. Formatting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files: clang/include/clang/CodeGen/CodeGenAction.h clang/include/clang/Frontend/FrontendAction.h

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

sgraenitz accepted this revision. sgraenitz added a comment. Thanks. From my side this looks good now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___ cfe-commits mailing list

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added inline comments. Comment at: clang/lib/Interpreter/Interpreter.cpp:93 + "Initialization failed. " + "Unable to create diagnostics engine"); + sgraenitz wrote: > It looks like

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 330381. v.g.vassilev marked 2 inline comments as done. v.g.vassilev added a comment. Address comments -- rename test file; add a logging diagnostic consumer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

sgraenitz added a comment. I think this makes good progress. I found two details in the test code that need attention. The stdout issue might be done now or in a follow-up patch some time soon. Otherwise, this seems to get ready to land. @teemperor What about your notes. Are there any open

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 329894. v.g.vassilev added a comment. Address most of the formatting suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files: clang/include/clang/CodeGen/CodeGenAction.h

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 329234. v.g.vassilev added a comment. - Do not rely on process exit code for `--host-supports-jit` but parse the true/false flag. - Move the `IncrementalProcessingTest` unittest from unittests/CodeGen to unittests/Interpreter. CHANGES SINCE LAST

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 329131. v.g.vassilev marked 2 inline comments as done. v.g.vassilev added a comment. Add a lit feature to check if llvm has jit support to selectively run tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rsmith added inline comments. Comment at: clang/test/Interpreter/execute.c:1 +// RUN: cat %s | clang-repl | FileCheck %s + Presumably here (and in all the interpreter tests) we will need to check that we configured Clang and LLVM so that they can actually JIT

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev marked 3 inline comments as done. v.g.vassilev added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908 +CodeGenerator *CodeGenAction::getCodeGenerator() const { + return BEConsumer->getCodeGenerator(); lhames wrote: >

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added a comment. > The other aspect of this is that upon unloading of these pieces of code we > need to run the destructors (that's why we need some non-canonical handling > of when we run the atexit handlers). I just noticed this comment. I think long term you could handle this by

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908 +CodeGenerator *CodeGenAction::getCodeGenerator() const { + return BEConsumer->getCodeGenerator(); lhames wrote: > v.g.vassilev wrote: > > sgraenitz wrote: > > >

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 325249. v.g.vassilev marked 7 inline comments as done. v.g.vassilev added a comment. Address Lang's comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files:

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added inline comments. Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184 + SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID()) + .getLocWithOffset(InputCount + 2); + teemperor wrote: > The `+

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 325071. v.g.vassilev marked 8 inline comments as done. v.g.vassilev added a comment. Address comments from Raphael and Stefan. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files:

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

lhames added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908 +CodeGenerator *CodeGenAction::getCodeGenerator() const { + return BEConsumer->getCodeGenerator(); v.g.vassilev wrote: > sgraenitz wrote: > > v.g.vassilev wrote: > > >

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added inline comments. Comment at: clang/lib/Interpreter/IncrementalParser.cpp:184 + SourceLocation NewLoc = SM.getLocForStartOfFile(SM.getMainFileID()) + .getLocWithOffset(InputCount + 2); + The `+ 2` here is probably not

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. Hi Stefan, Thanks a lot for the details you shared. They are really helpful to me. In D96033#2565708 , @sgraenitz wrote: > Hi Vassil, thanks for upstreaming this! I think it goes into a good direction. > > The last time I

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

sgraenitz added a subscriber: anarazel. sgraenitz added a comment. Hi Vassil, thanks for upstreaming this! I think it goes into a good direction. The last time I looked at the Cling sources downstream, it was based on LLVM release 5.0. The IncrementalJIT class was based on what we call OrcV1

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908 +CodeGenerator *CodeGenAction::getCodeGenerator() const { + return BEConsumer->getCodeGenerator(); @rjmccall, we were wondering if there is a better way to ask CodeGen to

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added inline comments. Comment at: clang/include/clang/Frontend/FrontendAction.h:238 /// objects, and run statistics and output file cleanup code. - void EndSourceFile(); + virtual void EndSourceFile(); teemperor wrote: > I think this and the

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 323531. v.g.vassilev marked 21 inline comments as done. v.g.vassilev added a comment. Address more review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files:

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

sgraenitz added a comment. @v.g.vassilev Great to see this getting upstreamed! @teemperor Thanks for adding, I will take a look in the next days. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 ___

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a reviewer: sgraenitz. teemperor added a comment. Another round of comments. Also a general note that applies to a bunch of places, if you specify the function names in comments when calling a function, you have to add a trailing `=` so that clang-tidy can actually check this

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev added a comment. @teemperor, thanks for the hints. Comment at: clang/include/clang/Interpreter/Transaction.h:1 +//===--- Transaction.h - Incremental Compilation and Execution---*- C++ -*-===// +// teemperor wrote: > Could this whole file just be

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev updated this revision to Diff 321544. v.g.vassilev marked 6 inline comments as done. v.g.vassilev added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96033/new/ https://reviews.llvm.org/D96033 Files:

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

teemperor added a comment. A more general comment: You have to `std::move` your `llvm::Error` variables when the result is `llvm::Expected` (otherwise this won't compile). Comment at: clang/include/clang/Interpreter/Transaction.h:1 +//===--- Transaction.h - Incremental

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

v.g.vassilev created this revision. v.g.vassilev added reviewers: rsmith, lhames, rjmccall, teemperor, aprantl. Herald added a subscriber: mgorny. v.g.vassilev requested review of this revision. In http://lists.llvm.org/pipermail/llvm-dev/2020-July/143257.html we have mentioned our plans to make