This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add time profile for constant evaluation
ClosedPublic

Authored by Izaron on Oct 15 2022, 10:25 AM.

Details

Summary

Add time profiler for various constexpr evaluation events
so that slow event could be visible on the visualized flame chart.

Diff Detail

Event Timeline

Izaron created this revision.Oct 15 2022, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 10:25 AM
Izaron requested review of this revision.Oct 15 2022, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2022, 10:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

We can use this file as an example: ConstProfiler.cpp https://reviews.llvm.org/P8296
The code does a really big constant evaluation.

This is the generated json before the patch: before.json https://reviews.llvm.org/P8294
Giving this json to https://www.speedscope.app/, we get a big empty gap where the constant evaluation is being done.
link to screenshot

This is the generated json after the patch: after.json https://reviews.llvm.org/P8295
The empty gap is filled with four events.
link to screenshot

You can use this command for generating the json:

clang++ ConstProfiler.cpp -std=c++20 -c -ftime-trace -ftime-trace-granularity=50

P.S. I'm not sure how to write tests to cover the patch, but I'll find this out =)

I like the idea of this — +1 for the direction. I'd like to see some tests through before I approve.

Thanks for working on this!

tbaeder added inline comments.
clang/lib/AST/ExprConstant.cpp
15277

what about Expr::EvaluateASRvalue and Expr::isPotentialConstantExpression?

Thank you for working on this, having more detailed timing information for where we're spending time in the frontend will really help us to evaluate where we need to work on frontend performance.

clang/lib/AST/ExprConstant.cpp
15277

Agreed; I think we should cover all the Expr::Evaluate* functions as well as related potentially expensive constant expression functions like Expr::isPotentialConstantExpression and Expr::isIntegerConstantExpr.

clang/lib/Sema/SemaDeclCXX.cpp
1740–1746 ↗(On Diff #468028)

Huh, I'm a bit surprised that the checking in this function isn't dominated by the time spent in Expr::isPotentialConstantExpr() -- if you add the time tracing to all of the constant expression evaluation functions, do you still need a time trace here in Sema?

clang/lib/Sema/SemaExpr.cpp
17544 ↗(On Diff #468028)

Why not push this down into EvaluateAsConstantExpr()? (Then the name you give the scope will also be more accurate.)

Izaron updated this revision to Diff 469096.Oct 19 2022, 6:07 PM
Izaron marked 4 inline comments as done.

Add time profiler test.

I'd like to see some tests through before I approve.

Thanks for the greenlight!

I added a test that compiles a chunk of code and then checks the time trace graph in a human-readable form.

clang/lib/Sema/SemaDeclCXX.cpp
1740–1746 ↗(On Diff #468028)

Thanks! Removed trace here, added to Expr::isPotentialConstantExpr(). This method is called inside of CheckConstexprFunctionDefinition and really takes almost all the time.

clang/lib/Sema/SemaExpr.cpp
17544 ↗(On Diff #468028)

Thanks! I wrote on this level because I didn't manage to get SemaRef.getSourceManager() without SemaRef (we pass down SemaRef.getASTContext()).

Anyway I found out that we can use SemaRef.getASTContext().getSourceManager() =)

The test doesn't cover exactly all new traces though. For example I couldn't write a code that runs into the EvaluateAsInt method 🤔

If you have an idea for some funky constexpr code that can be tested, please write here =)

Izaron updated this revision to Diff 469098.Oct 19 2022, 6:16 PM

Fix CMakeLists.txt

Izaron updated this revision to Diff 469159.Oct 20 2022, 3:59 AM

Fix optionals with value_or

aaron.ballman added inline comments.Oct 20 2022, 11:00 AM
clang/unittests/Support/TimeProfilerTest.cpp
198–199

This bit worries me because I suspect we'll get pretty wide variation between test bots in the build lab. Do you have an idea of how likely it is that this test will have different behavior depending on the machine?

Izaron added inline comments.Oct 20 2022, 11:19 AM
clang/unittests/Support/TimeProfilerTest.cpp
198–199

I'd say, the test's outcome varies if behaviour of CompilerInstance varies. From my (imperfect) understanding of Clang/LLVM's architecture, its interface is pretty hermetic and we won't get inconsistent behaviour depending on the current machine as soon as we ran Compiler.ExecuteAction(Action);.
Since the code is machine-independents, I expect the same behaviour everywhere.

If we look on other tests, there are tests that seemingly take the machine's target triple https://github.com/llvm/llvm-project/blob/3fee9358baab54e4ed646a106297e7fb6f1b4cff/clang/unittests/CodeGen/TestCompiler.h#L40-L48 to fill some info in CompilerInstance.

But this is not the case in our test. Seems like the AST parsing is machine-independent 😃

aaron.ballman accepted this revision.Oct 21 2022, 10:56 AM

LGTM! Please be sure to add a release note to alert folks to the time profiler becoming more awesome. :-)

clang/unittests/Support/TimeProfilerTest.cpp
198–199

Excellent, thank you for the explanation!

This revision is now accepted and ready to land.Oct 21 2022, 10:56 AM
Izaron updated this revision to Diff 469771.Oct 21 2022, 2:32 PM

Mention this in the release notes. Thanks to Aaron for reviewing!

This revision was landed with ongoing or failed builds.Oct 21 2022, 4:25 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 21 2022, 5:21 PM
thakis added inline comments.
clang/unittests/Support/TimeProfilerTest.cpp
12

Why is this in clang/unittests/Support (a new binary to boot)? This doesn't use any code form clang/lib/Support as far as I can tell.

Izaron added inline comments.Oct 21 2022, 5:29 PM
clang/unittests/Support/TimeProfilerTest.cpp
12

It uses code from llvm/Support.

There are already tests with similar names: clang/unittest/CodeGen and llvm/unittests/CodeGen, both are pretty close in spirit.

So I decided to make clang/unittests/Support as a counterpart to llvm/unittests/Support. There is a time profiler test too - https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Support/TimeProfilerTest.cpp

dyung added a subscriber: dyung.Oct 22 2022, 7:13 PM

Hi @Izaron, several of our internal tests that run tests using -ftime-trace started crashing when run which I bisected back to your change. I have filed issue #58551 for it, can you take a look?

Hi @Izaron, several of our internal tests that run tests using -ftime-trace started crashing when run which I bisected back to your change. I have filed issue #58551 for it, can you take a look?

Hi! Thank you for filling the issue! I will fix it as soon as I can!

Thanks for this!
When I experimented with this for #42754 I also came across functions like
Expr::isConstantInitializer
ExprConstant.cpp HandleFunctionCall/HandleConstructorCall
ByteCodeEmitter::compileFunc

Worth checking out if these are already covered or potentially useful.