Page MenuHomePhabricator

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

Authored by aeubanks on Oct 4 2021, 4:56 PM.

Details

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.

Diff Detail

Event Timeline

aeubanks created this revision.Oct 4 2021, 4:56 PM
aeubanks requested review of this revision.Oct 4 2021, 4:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2021, 4:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dblaikie accepted this revision.Oct 5 2021, 12:23 PM

Seems like a good place to start with experimentation.

This revision is now accepted and ready to land.Oct 5 2021, 12:23 PM
dblaikie requested changes to this revision.Oct 5 2021, 12:26 PM
dblaikie added a subscriber: aaron.ballman.

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

This revision now requires changes to proceed.Oct 5 2021, 12:26 PM
aeubanks updated this revision to Diff 377333.Oct 5 2021, 1:02 PM

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

dblaikie accepted this revision.Oct 5 2021, 7:30 PM

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.

This revision is now accepted and ready to land.Oct 5 2021, 7:30 PM
aeubanks added inline comments.Oct 6 2021, 1:38 PM
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.

This revision was landed with ongoing or failed builds.Oct 6 2021, 1:42 PM
This revision was automatically updated to reflect the committed changes.

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

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.

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