Teach codegen to work in incremental processing mode.

[v.g.vassilev]

Summary

When isIncrementalProcessingEnabled is on we might call multiple times HandleEndOfTranslationUnit. This would complete the llvm::Module CodeGen is writing to.

This patch allows the clients to start a new llvm::Module, allowing CodeGen to continue writing.

This should give the necessary facilities to write a unittest for D34059.

[v.g.vassilev]

Fix compilation issue.

[v.g.vassilev]

Add a test.

Patch by Axel Naumann!

[v.g.vassilev]

Bring back the initial state of this patch. I accidentally uploaded a wrong patch here, instead of its dependee...

[rsmith]

This makes sense to me, but I'd like John's input on whether this is a reasonable facility for CodeGen to have, and whether this is sufficient for (for example) inline function definitions to be emitted at the right times into the right Modules. (The tests for this appear to be in D34059.)

[rjmccall]

What's the relationship between these llvm::Modules that you want to generate? Are you imagining them as separate translation units, or are the subsequent modules more like addenda to the original?

[v.g.vassilev]

We use them as separate translation units due to the fact that we need to survive multiple calls to HandleEndOfTranslationUnit. This 'finalizes' the module, and we cannot write to it anymore. Even though we could write to it (this was the case when we were using the old JIT), with the orc infrastructure we generate machine code for the whole module.

[rjmccall]

Okay. In that case, I see two problems, one major and one potentially major.

The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in later CGMs. Probably the easiest way of doing that is to eliminate CodeGenModule's dependence on having on-demand definitions pre-registered with it: that is, if it emits a reference to a declaration whose definition already exists in the AST, CodeGenModule should use that definition even if e.g. HandleTopLevelDecl has not been called. Calling HandleTopLevelDecl would only be necessary if the definition is discovered/instantiated after code is generated for the use. I see little advantage to the current rule, and doing this would also be a nice quality-of-life improvement for various out-of-tree projects (including Swift, but IIRC there are several others) which deserialize references to a complete AST, and which currently have to walk the definition and recursively register any definitions used within.

The potentially major problem is that it is not possible in general to automatically break up a single translation unit into multiple translation units, for two reasons. The big reason is that there is no way to correctly handle entities with non-external linkage that are referenced from two parts of the translation unit without implicitly finding some way to promote them to external linkage, which might open a huge can of worms if you have multiple "outer" translation units. The lesser reason is that the prefix of a valid translation unit is not necessarily a valid translation unit: for example, a static or inline function can be defined at an arbitrary within the translation unit, i.e. not necessarily before its first use. But if your use case somehow defines away these problems, this might be fine.

As a minor request, if you are going to make HandleTranslationUnit no longer the final call on CodeGenerator, please either add a new method that *is* a guaranteed final call or add a new method that does the whole "end a previous part of the translation unit and start a new one" step.

lib/CodeGen/ModuleBuilder.cpp

328

Why does this one take a const std::string & instead of a StringRef?

[v.g.vassilev]

@rjmccall, thanks for the prompt and thorough reply.

In D34444#793311, @rjmccall wrote:

Okay. In that case, I see two problems, one major and one potentially major.

This is a very accurate diagnosis which took us 5 years to discover on an empirical basis ;)

The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in later CGMs.

We bring the CGM state to the subsequent CGMs. See https://github.com/vgvassilev/clang/blob/cling-patches/lib/CodeGen/ModuleBuilder.cpp#L138-L160

The potentially major problem is that it is not possible in general to automatically break up a single translation unit into multiple translation units, for two reasons. The big reason is that there is no way to correctly handle entities with non-external linkage that are referenced from two parts of the translation unit without implicitly finding some way to promote them to external linkage, which might open a huge can of worms if you have multiple "outer" translation units.

We do not have an multiple 'outer' translation units. We have just one ever growing TU (which probably invalidates my previous statement that we have a distinct TUs) which we send to the RuntimeDyLD allowing only JIT to resolve symbols from it.  We aid the JIT when resolving symbols with internal linkage by changing all internal linkage to external (We haven't seen issues with that approach).

The lesser reason is that the prefix of a valid translation unit is not necessarily a valid translation unit: for example, a static or inline function can be defined at an arbitrary within the translation unit, i.e. not necessarily before its first use. But if your use case somehow defines away these problems, this might be fine.

If we end up with a module containing no definition of a symbol and such is required, then we complain. So indeed we are defining away this issue.

As a minor request, if you are going to make HandleTranslationUnit no longer the final call on CodeGenerator, please either add a new method that *is* a guaranteed final call or add a new method that does the whole "end a previous part of the translation unit and start a new one" step.

We can have this but it would be a copy of `HandleTranslationUnit`.  The `StartModule` interface is the antagonist routine to `ReleaseModule`. If you prefer we could merge `StartModule` into `ReleaseModule` adding a flag (or reading the value of `isIncrementalProcessingEnabled`).

(Thanks @karies for helping!)

[v.g.vassilev]

ping...

[rjmccall]

In D34444#795175, @v.g.vassilev wrote:

@rjmccall, thanks for the prompt and thorough reply.

In D34444#793311, @rjmccall wrote:

Okay. In that case, I see two problems, one major and one potentially major.

This is a very accurate diagnosis which took us 5 years to discover on an empirical basis ;)

You could've asked at any time. :)

The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in later CGMs.

We bring the CGM state to the subsequent CGMs. See https://github.com/vgvassilev/clang/blob/cling-patches/lib/CodeGen/ModuleBuilder.cpp#L138-L160

That's quite brittle, because that code is only executed in a code path that only you are using, and you're not adding any tests. I would greatly prefer a change to IRGen's core assumptions, as suggested.

The potentially major problem is that it is not possible in general to automatically break up a single translation unit into multiple translation units, for two reasons. The big reason is that there is no way to correctly handle entities with non-external linkage that are referenced from two parts of the translation unit without implicitly finding some way to promote them to external linkage, which might open a huge can of worms if you have multiple "outer" translation units.

We do not have an multiple 'outer' translation units. We have just one ever growing TU (which probably invalidates my previous statement that we have a distinct TUs) which we send to the RuntimeDyLD allowing only JIT to resolve symbols from it.  We aid the JIT when resolving symbols with internal linkage by changing all internal linkage to external (We haven't seen issues with that approach).

Ah, okay. Yes, that is a completely different translation model from having distinct TUs.

IRGen will generally happily emit references to undefined internal objects; instead of hacking the linkage, you could just clean that up as a post-pass. Although hacking the linkage as post-pass is reasonable, too. In either case, you can recognize uses of internal-linkage objects that haven't been defined yet and report that back to the user.

The lesser reason is that the prefix of a valid translation unit is not necessarily a valid translation unit: for example, a static or inline function can be defined at an arbitrary within the translation unit, i.e. not necessarily before its first use. But if your use case somehow defines away these problems, this might be fine.

If we end up with a module containing no definition of a symbol and such is required, then we complain. So indeed we are defining away this issue.

Ok.

As a minor request, if you are going to make HandleTranslationUnit no longer the final call on CodeGenerator, please either add a new method that *is* a guaranteed final call or add a new method that does the whole "end a previous part of the translation unit and start a new one" step.

We can have this but it would be a copy of `HandleTranslationUnit`.  The `StartModule` interface is the antagonist routine to `ReleaseModule`. If you prefer we could merge `StartModule` into `ReleaseModule` adding a flag (or reading the value of `isIncrementalProcessingEnabled`).

I feel it is important that there be a way to inform an ASTConsumer that no further requests will be made of it, something other than calling its destructor. I would like you to make sure that the ASTConsumer interface supports that and that that call is not made too soon in your alternate processing mode.

[v.g.vassilev]

In D34444#812418, @rjmccall wrote:

In D34444#795175, @v.g.vassilev wrote:

@rjmccall, thanks for the prompt and thorough reply.

In D34444#793311, @rjmccall wrote:

Okay. In that case, I see two problems, one major and one potentially major.

This is a very accurate diagnosis which took us 5 years to discover on an empirical basis ;)

You could've asked at any time. :)

True. I am not really sure I knew what to ask, though ;)

The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in later CGMs.

We bring the CGM state to the subsequent CGMs. See https://github.com/vgvassilev/clang/blob/cling-patches/lib/CodeGen/ModuleBuilder.cpp#L138-L160

That's quite brittle, because that code is only executed in a code path that only you are using, and you're not adding any tests. I would greatly prefer a change to IRGen's core assumptions, as suggested.

I am open to changing this code as well. That should probably be another review.

The potentially major problem is that it is not possible in general to automatically break up a single translation unit into multiple translation units, for two reasons. The big reason is that there is no way to correctly handle entities with non-external linkage that are referenced from two parts of the translation unit without implicitly finding some way to promote them to external linkage, which might open a huge can of worms if you have multiple "outer" translation units.

We do not have an multiple 'outer' translation units. We have just one ever growing TU (which probably invalidates my previous statement that we have a distinct TUs) which we send to the RuntimeDyLD allowing only JIT to resolve symbols from it.  We aid the JIT when resolving symbols with internal linkage by changing all internal linkage to external (We haven't seen issues with that approach).

Ah, okay. Yes, that is a completely different translation model from having distinct TUs.

IRGen will generally happily emit references to undefined internal objects; instead of hacking the linkage, you could just clean that up as a post-pass. Although hacking the linkage as post-pass is reasonable, too. In either case, you can recognize uses of internal-linkage objects that haven't been defined yet and report that back to the user.

The lesser reason is that the prefix of a valid translation unit is not necessarily a valid translation unit: for example, a static or inline function can be defined at an arbitrary within the translation unit, i.e. not necessarily before its first use. But if your use case somehow defines away these problems, this might be fine.

If we end up with a module containing no definition of a symbol and such is required, then we complain. So indeed we are defining away this issue.

Ok.

As a minor request, if you are going to make HandleTranslationUnit no longer the final call on CodeGenerator, please either add a new method that *is* a guaranteed final call or add a new method that does the whole "end a previous part of the translation unit and start a new one" step.

We can have this but it would be a copy of `HandleTranslationUnit`.  The `StartModule` interface is the antagonist routine to `ReleaseModule`. If you prefer we could merge `StartModule` into `ReleaseModule` adding a flag (or reading the value of `isIncrementalProcessingEnabled`).

I feel it is important that there be a way to inform an ASTConsumer that no further requests will be made of it, something other than calling its destructor. I would like you to make sure that the ASTConsumer interface supports that and that that call is not made too soon in your alternate processing mode.

Do you have a preference of a name of this new interface?

[rjmccall]

In D34444#812836, @v.g.vassilev wrote:

In D34444#812418, @rjmccall wrote:

In D34444#795175, @v.g.vassilev wrote:

@rjmccall, thanks for the prompt and thorough reply.

In D34444#793311, @rjmccall wrote:

Okay. In that case, I see two problems, one major and one potentially major.

This is a very accurate diagnosis which took us 5 years to discover on an empirical basis ;)

You could've asked at any time. :)

True. I am not really sure I knew what to ask, though ;)

We're open to general "I'm trying to do this and having problems" questions on the mailing lists. You probably would've needed to know to CC me specifically, though; sadly, I can't keep up with all the lists I need to.

That's quite brittle, because that code is only executed in a code path that only you are using, and you're not adding any tests. I would greatly prefer a change to IRGen's core assumptions, as suggested.

I am open to changing this code as well. That should probably be another review.

I agree. Are you comfortable with blocking this review until that lands? It seems like it would significantly change this.

I feel it is important that there be a way to inform an ASTConsumer that no further requests will be made of it, something other than calling its destructor. I would like you to make sure that the ASTConsumer interface supports that and that that call is not made too soon in your alternate processing mode.

Do you have a preference of a name of this new interface?

Maybe just "finish"?

John.

[v.g.vassilev]

In D34444#818030, @rjmccall wrote:

In D34444#812836, @v.g.vassilev wrote:

In D34444#812418, @rjmccall wrote:

In D34444#795175, @v.g.vassilev wrote:

@rjmccall, thanks for the prompt and thorough reply.

In D34444#793311, @rjmccall wrote:

Okay. In that case, I see two problems, one major and one potentially major.

This is a very accurate diagnosis which took us 5 years to discover on an empirical basis ;)

You could've asked at any time. :)

True. I am not really sure I knew what to ask, though ;)

We're open to general "I'm trying to do this and having problems" questions on the mailing lists. You probably would've needed to know to CC me specifically, though; sadly, I can't keep up with all the lists I need to.

Good to know. Thanks! I will come back to you once I get rid of our O(100) clang patches to discuss what would be the best way of supporting incremental compilation.

That's quite brittle, because that code is only executed in a code path that only you are using, and you're not adding any tests. I would greatly prefer a change to IRGen's core assumptions, as suggested.

I am open to changing this code as well. That should probably be another review.

I agree. Are you comfortable with blocking this review until that lands? It seems like it would significantly change this.

I am afraid that will slow down (if not suspend) our efforts to upstream our local patches. This patch is pretty fundamental for cling and if we change it now, I will have to go back and rework our implementation. I'd be much more comfortable in reworking it once we run on vanilla clang (then efforts seems easier to justify on our end).

It seems to me that despite being suboptimal, it is not very intrusive and it would effect only on our use case. I can keep track of such patches and come back to you for advice how to do them best. Would that make sense?

I feel it is important that there be a way to inform an ASTConsumer that no further requests will be made of it, something other than calling its destructor. I would like you to make sure that the ASTConsumer interface supports that and that that call is not made too soon in your alternate processing mode.

Do you have a preference of a name of this new interface?

Maybe just "finish"?

John.

[rjmccall]

In D34444#818047, @v.g.vassilev wrote:

I am open to changing this code as well. That should probably be another review.

I agree. Are you comfortable with blocking this review until that lands? It seems like it would significantly change this.

I am afraid that will slow down (if not suspend) our efforts to upstream our local patches. This patch is pretty fundamental for cling and if we change it now, I will have to go back and rework our implementation. I'd be much more comfortable in reworking it once we run on vanilla clang (then efforts seems easier to justify on our end).

It seems to me that despite being suboptimal, it is not very intrusive and it would effect only on our use case. I can keep track of such patches and come back to you for advice how to do them best. Would that make sense?

My concern is that efforts to upstream patches are doomed to get bogged down anyway, and in the meantime we'll have however much more untestable code. But we have some of that anyway, so it's at least not novel. I'm willing to accept it.

John.

[v.g.vassilev]

std::string& -> llvm::StringRef.

[v.g.vassilev]

Type& name -> Type &name.

[v.g.vassilev]

A version of this landed in r311843. I am keeping in mind this discussion and I'd like to follow up with @rjmccall once I open the more major review item (libInterpreter).

[v.g.vassilev]

This landed slightly modified here: https://github.com/llvm/llvm-project/commit/4d54e543abd5d0a8b0a321f8c292252f4895693a

include/clang/CodeGen/ModuleBuilder.h

public:
   ///
   /// \param isForDefinition If true, the caller intends to define the
   ///   entity; the object returned will be an llvm::GlobalValue of
   ///   some sort.  If false, the caller just intends to use the entity;
   ///   the object returned may be any sort of constant value, and the
   ///   code generator will schedule the entity for emission if a
   ///   definition has been registered with this code generator.
   llvm::Constant *GetAddrOfGlobal(GlobalDecl decl, bool isForDefinition);
+
+  /// Create a new \c llvm::Module after calling HandleTranslationUnit. This
+  /// enable codegen in interactive processing environments.
+  llvm::Module* StartModule(llvm::StringRef ModuleName, llvm::LLVMContext &C,
+                            const CodeGenOptions &CGO);
 };
 
 /// CreateLLVMCodeGen - Create a CodeGenerator instance.
 /// It is the responsibility of the caller to call delete on
 /// the allocated CodeGenerator instance.
 CodeGenerator *CreateLLVMCodeGen(DiagnosticsEngine &Diags,
                                  llvm::StringRef ModuleName,
                                  const HeaderSearchOptions &HeaderSearchOpts,
                                  const PreprocessorOptions &PreprocessorOpts,
                                  const CodeGenOptions &CGO,
                                  llvm::LLVMContext& C,
                                  CoverageSourceInfo *CoverageInfo = nullptr);
 
 } // end namespace clang
 
 #endif

lib/CodeGen/ModuleBuilder.cpp

const Decl *GetDeclForMangledName(StringRef MangledName) {
       }
       return D;
     }
 
     llvm::Constant *GetAddrOfGlobal(GlobalDecl global, bool isForDefinition) {
       return Builder->GetAddrOfGlobal(global, ForDefinition_t(isForDefinition));
     }
 
+    llvm::Module *StartModule(llvm::StringRef ModuleName, llvm::LLVMContext &C,
+                              const CodeGenOptions &CGO) {
+      assert(!M && "Replacing existing Module?");
+      M.reset(new llvm::Module(ModuleName, C));
+      Initialize(*Ctx);
+      return M.get();
+    }
+
     void Initialize(ASTContext &Context) override {
       Ctx = &Context;
 
       M->setTargetTriple(Ctx->getTargetInfo().getTriple().getTriple());
       M->setDataLayout(Ctx->getTargetInfo().getDataLayout());
       Builder.reset(new CodeGen::CodeGenModule(Context, HeaderSearchOpts,
                                                PreprocessorOpts, CodeGenOpts,
                                                *M, Diags, CoverageInfo));

 }
 
 llvm::Constant *CodeGenerator::GetAddrOfGlobal(GlobalDecl global,
                                                bool isForDefinition) {
   return static_cast<CodeGeneratorImpl*>(this)
            ->GetAddrOfGlobal(global, isForDefinition);
 }
 
+llvm::Module *CodeGenerator::StartModule(llvm::StringRef ModuleName,
+                                         llvm::LLVMContext &C) {
+   return static_cast<CodeGeneratorImpl*>(this)->StartModule(ModuleName, C);
+}
+
 CodeGenerator *clang::CreateLLVMCodeGen(
     DiagnosticsEngine &Diags, llvm::StringRef ModuleName,
     const HeaderSearchOptions &HeaderSearchOpts,
     const PreprocessorOptions &PreprocessorOpts, const CodeGenOptions &CGO,
     llvm::LLVMContext &C, CoverageSourceInfo *CoverageInfo) {
   return new CodeGeneratorImpl(Diags, ModuleName, HeaderSearchOpts,
                                PreprocessorOpts, CGO, C, CoverageInfo);
 }