This is an archive of the discontinued LLVM Phabricator instance.

[Lex] Keep track of skipped preprocessor blocks and advance the lexer directly if they are revisited
ClosedPublic

Authored by akyrtzi on Jun 8 2022, 10:32 PM.

Details

Summary

This speeds up preprocessing, specifically for preprocessing the clang sources time is reduced by about -36%,
using measurements on M1Pro with a release+thinLTO build.

Diff Detail

Event Timeline

akyrtzi created this revision.Jun 8 2022, 10:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 10:32 PM
akyrtzi requested review of this revision.Jun 8 2022, 10:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 10:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
benlangmuir added inline comments.Jun 13 2022, 10:08 AM
clang/lib/Lex/PPDirectives.cpp
520

Storing this pointer is only safe as long as SkipExcludedConditionalBlock can never be called recursively, since any modification to the DenseMap can invalidate this pointer. Is there some cheap way to assert that? If not, maybe we should do the lookup again in endLexPass, which would always be correct.

akyrtzi added inline comments.Jun 13 2022, 11:42 AM
clang/lib/Lex/PPDirectives.cpp
520

Good point! I can add a bool to assert that SkipExcludedConditionalBlock is not recursively called.

akyrtzi updated this revision to Diff 436554.Jun 13 2022, 1:55 PM

Assert that SkipExcludedConditionalBlock() is not recursively called.

akyrtzi marked an inline comment as done.Jun 13 2022, 1:56 PM
akyrtzi added inline comments.
clang/lib/Lex/PPDirectives.cpp
520

Done 👍

benlangmuir accepted this revision.Jun 13 2022, 2:09 PM

Couple more minor things, but basically LGTM.

clang/include/clang/Lex/Preprocessor.h
2601

Would be good to mention *why* this is unsafe so we don't remove it by accident (or so we *can* remove it later if we ever change the code so it doesn't matter anymore).

Oh, and style nit: we should move these new fields to the top of the class where all the other fields are.

This revision is now accepted and ready to land.Jun 13 2022, 2:09 PM
akyrtzi updated this revision to Diff 436612.Jun 13 2022, 6:18 PM

Add more comments about the use of SkippingExcludedConditionalBlock and move the new Preprocessor fields towards the top of the class.

This revision was landed with ongoing or failed builds.Jun 13 2022, 9:46 PM
This revision was automatically updated to reflect the committed changes.

Do you believe an entry in the ReleaseNotes would get this achievement more visibility?

tambre added a subscriber: tambre.Jun 14 2022, 4:20 AM

Do you believe an entry in the ReleaseNotes would get this achievement more visibility?

I second that this would be worthy of a release note. Seems like quite a big improvement.

I wonder how much is the total speedup for a full LLVM build or such?