[clang] Add option to clear AST memory before running LLVM passes

[aeubanks]

Summary

This is to save memory for Clang compiles.
Measuring building PassBuilder.cpp under /usr/bin/time, max rss goes from 0.93GB to 0.7GB.

This does not turn it by default yet.

I've turned on the option locally and run it over a good amount of files without any issues.

For more background, see
https://lists.llvm.org/pipermail/cfe-dev/2021-September/068930.html.

[dblaikie]

Seems like a good place to start with experimentation.

[dblaikie]

Oh, looking at this ab it further I do have some things to discuss.

clang/include/clang/Driver/Options.td

5300–5302

I think you might have to flag this as a NonDriverOption otherwise it'd be accepted as a driver option, when I guess it's only intended as a CC1 option? (looks like lots of other options have this mistake, unfortrunately - @aaron.ballman )

clang/test/Misc/clear-ast-before-backend.c

3–4

This is a driver test, but not a very interesting one - it tests that -Xclang arguments are passed directyl to cc1, which is probably tested elsewhere already?

I'm not sure there's anything we could really test with the first RUN line - though since it's not a Driver test and doesn't need to be a Driver test - it should probably just test cc1 -clear-ast-before-backend directly rather than going through the driver+-Xclang

[aeubanks]

update test
I checked to make sure that we're not accepting -clear-ast-before-backend as a driver flag

[dblaikie]

In D111105#3043734, @aeubanks wrote:

update test
I checked to make sure that we're not accepting -clear-ast-before-backend as a driver flag

Ah, OK - sorry I mistested some things when I was thinking about that.

clang/test/Misc/clear-ast-before-backend.c

3–4

Might be worth fleshing this out somehow to observe some behavior? I'm not sure exactly how - not like we'd generally test other memory optimizations we might make without a flag to opt into them. But perhaps a simple Hello World, maybe with some optimizations applied (so making this one of those rare "actually check it going through some optimizations" - I guess ever -O0 with an always_inline function or the like to demonstrate that the optimizations were applied, etc)

Can't think of much else that wouldn't just be obnoxious in terms of memory usage - oh, unless there's some way to set an artificially low memory limit (some other tests might do this - clang/test/PCH/leakfiles.test puts a low ulimit for instance - some lldb and compiler-rt tests also set ulimits of various kinds to observe failures). An A/B test - showing some modest example hitting the memory limit without the flag, but passing with the flag, could be nice to have.

[aeubanks]

clang/test/Misc/clear-ast-before-backend.c

3–4

This is similar to perf testing which we don't really have in tree tests for. Typically these things are mostly monitored separately (e.g. llvm-compile-time-tracker).
As for making sure that optimizations still happen, -emit-obj requires that. If that isn't happening with -emit-obj then something is very wrong. I'll add a function and make this -O1 though so we test more passes.

[aeubanks]

update

[dblaikie]

This is similar to perf testing which we don't really have in tree tests for. Typically these things are mostly monitored separately (e.g. llvm-compile-time-tracker).

Except in this case it isn't tested at all because it's behind a flag. Unless we're bringing up a buildbot/tracker that tracks this configuration?

As for making sure that optimizations still happen, -emit-obj requires that. If that isn't happening with -emit-obj then something is very wrong. I'll add a function and make this -O1 though so we test more passes.

I think checking the IR would be worthwhile too, otherwise this is still a "does anything other than crash" test, which I think is a bit too broad of a test.

clang/include/clang/Driver/Options.td

5300–5302

Except in this case it isn't monitored at all

[aeubanks]

In D111105#3046585, @dblaikie wrote:

This is similar to perf testing which we don't really have in tree tests for. Typically these things are mostly monitored separately (e.g. llvm-compile-time-tracker).

Except in this case it isn't tested at all because it's behind a flag. Unless we're bringing up a buildbot/tracker that tracks this configuration?

It'll be on by default soon, bringing up a separate tracker in the meantime is not worth it. I've already run llvm-compile-time-tracker before with this turned on with good results.

As for making sure that optimizations still happen, -emit-obj requires that. If that isn't happening with -emit-obj then something is very wrong. I'll add a function and make this -O1 though so we test more passes.

I think checking the IR would be worthwhile too, otherwise this is still a "does anything other than crash" test, which I think is a bit too broad of a test.

I still don't think that is useful at all, this code doesn't touch that sort of stuff so that's more of an unrelated thing.
Given that we're about to turn this on by default, I think it's much more useful to run this over a large codebase, see if anything crashes, and add those as regression tests.

[dblaikie]

In D111105#3046631, @aeubanks wrote:

In D111105#3046585, @dblaikie wrote:

This is similar to perf testing which we don't really have in tree tests for. Typically these things are mostly monitored separately (e.g. llvm-compile-time-tracker).

Except in this case it isn't tested at all because it's behind a flag. Unless we're bringing up a buildbot/tracker that tracks this configuration?

It'll be on by default soon, bringing up a separate tracker in the meantime is not worth it. I've already run llvm-compile-time-tracker before with this turned on with good results.

As for making sure that optimizations still happen, -emit-obj requires that. If that isn't happening with -emit-obj then something is very wrong. I'll add a function and make this -O1 though so we test more passes.

I think checking the IR would be worthwhile too, otherwise this is still a "does anything other than crash" test, which I think is a bit too broad of a test.

I still don't think that is useful at all, this code doesn't touch that sort of stuff so that's more of an unrelated thing.
Given that we're about to turn this on by default, I think it's much more useful to run this over a large codebase, see if anything crashes, and add those as regression tests.

Fair enough

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(DataSections      , 1, 0) ///< Set when -fdata-sections is enabled.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.
 CODEGENOPT(UniqueBasicBlockSectionNames, 1, 1) ///< Set for -funique-basic-block-section-names,
                                                ///< Produce unique section names with
                                                ///< basic block sections.
 CODEGENOPT(EnableAIXExtendedAltivecABI, 1, 0) ///< Set for -mabi=vec-extabi. Enables the extended Altivec ABI on AIX.
 ENUM_CODEGENOPT(FramePointer, FramePointerKind, 2, FramePointerKind::None) /// frame-pointer: all,non-leaf,none
 
+CODEGENOPT(ClearASTBeforeBackend , 1, 0) ///< Free the AST before running backend code generation. Only works with -disable-free.
 CODEGENOPT(DisableFree       , 1, 0) ///< Don't free memory.
 CODEGENOPT(DiscardValueNames , 1, 0) ///< Discard Value Names from the IR (LLVMContext flag)
 CODEGENOPT(DisableLLVMPasses , 1, 0) ///< Don't run any LLVM IR passes to get
                                      ///< the pristine IR generated by the
                                      ///< frontend.
 CODEGENOPT(DisableLifetimeMarkers, 1, 0) ///< Don't emit any lifetime markers
 CODEGENOPT(DisableO0ImplyOptNone , 1, 0) ///< Don't annonate function with optnone at O0
 CODEGENOPT(ExperimentalStrictFloatingPoint, 1, 0) ///< Enables the new, experimental

clang/include/clang/Driver/Options.td

def code_completion_brief_comments : Flag<["-"], "code-completion-brief-comments">,
   HelpText<"Include brief documentation comments in code-completion results.">,
   MarshallingInfoFlag<FrontendOpts<"CodeCompleteOpts.IncludeBriefComments">>;
 def code_completion_with_fixits : Flag<["-"], "code-completion-with-fixits">,
   HelpText<"Include code completion results which require small fix-its.">,
   MarshallingInfoFlag<FrontendOpts<"CodeCompleteOpts.IncludeFixIts">>;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">,
   MarshallingInfoFlag<FrontendOpts<"DisableFree">>;
+def clear_ast_before_backend : Flag<["-"], "clear-ast-before-backend">,
+  HelpText<"Clear the Clang AST before running backend code generation">,
+  MarshallingInfoFlag<CodeGenOpts<"ClearASTBeforeBackend">>;
 def enable_noundef_analysis : Flag<["-"], "enable-noundef-analysis">, Group<f_Group>,
   HelpText<"Enable analyzing function argument and return types for mandatory definedness">,
   MarshallingInfoFlag<CodeGenOpts<"EnableNoundefAttrs">>;
 def discard_value_names : Flag<["-"], "discard-value-names">,
   HelpText<"Discard value names in LLVM IR">,
   MarshallingInfoFlag<CodeGenOpts<"DiscardValueNames">>;
 def plugin_arg : JoinedAndSeparate<["-"], "plugin-arg-">,
     MetaVarName<"<name> <arg>">,

clang/lib/CodeGen/CodeGenAction.cpp

void HandleTranslationUnit(ASTContext &C) override {
         if (const Decl *FD = Gen->GetDeclForMangledName(F.getName())) {
           auto Loc = FD->getASTContext().getFullLoc(FD->getLocation());
           // TODO: use a fast content hash when available.
           auto NameHash = llvm::hash_value(F.getName());
           ManglingFullSourceLocs.push_back(std::make_pair(NameHash, Loc));
         }
       }
 
+      // FIXME: Fix cleanup issues with clearing the AST when we properly free
+      // things.
+      if (CodeGenOpts.DisableFree && CodeGenOpts.ClearASTBeforeBackend)
+        C.getAllocator().Reset();
+
       EmbedBitcode(getModule(), CodeGenOpts, llvm::MemoryBufferRef());
 
       EmitBackendOutput(Diags, HeaderSearchOpts, CodeGenOpts, TargetOpts,
                         LangOpts, C.getTargetInfo().getDataLayoutString(),
                         getModule(), Action, std::move(AsmOutStream));
 
       Ctx.setDiagnosticHandler(std::move(OldDiagnosticHandler));
 

clang/test/Misc/clear-ast-before-backend.c

+// RUN: %clang_cc1 -clear-ast-before-backend %s -emit-obj -o /dev/null -O1
+
+void f() {}