Page MenuHomePhabricator

[Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature
ClosedPublic

Authored by avt77 on Apr 13 2018, 5:56 AM.

Details

Summary

To simplify the review and commit of D43578 I decided to spilt it in several small parts.

This patch is the first part of the new series of patches. This patch introduces a dedicated boolean to deal with -ftime-report Clang switch (instead of 'llvm::TimePassesIsEnabled' which was LLVM dependent and potentionally could increase Clang compilation time).

Next patch will show usage of llvm::NamedRegionTimer instead of llvm::TimeRegion - it will really improve the generating output of the given feature.

Then I'll show the new class to deal with reqursive time counters (instead of approach shown in CodeGenAction.cpp).

And then we'll try new time counters (Preprocessor, Include Files, Parsing, Sema, CodeGen, etc.) The final list of counters will grow in dpendence of possible future requirements.

Finally, I'm going to introduce the feature which is similar to this one.

Hope, all these efforts could be interesting for many of us.

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Apr 13 2018, 5:56 AM

This makes sense.

include/clang/Frontend/Utils.h
241 ↗(On Diff #142383)

Don't really like global variables, but I guess timers are global state anyway, so this isn't really making anything worse.

lib/Basic/FrontendTiming.cpp
18 ↗(On Diff #142383)

Why is this in lib/Basic, when the declaration is in include/clang/Frontend/?

avt77 added inline comments.Apr 14 2018, 12:01 AM
lib/Basic/FrontendTiming.cpp
18 ↗(On Diff #142383)

Because this library is being linked to all others and as result this global variable could be seen in any Clang library w/o any changes in config files. Or you mean it should be moved from Frontend? But where? It's a frontend feature that's why I put its declaration there.

efriedma added inline comments.Apr 16 2018, 12:57 PM
lib/Basic/FrontendTiming.cpp
18 ↗(On Diff #142383)

The include structure should match the library structure; if the definition needs to be in lib/Basic/, the declaration should be in include/clang/Basic.

avt77 updated this revision to Diff 142755.Apr 17 2018, 4:19 AM

I moved FrontendTiming.cpp from lib/Basic/ to lib/Frontend/.

avt77 added a comment.Apr 19 2018, 1:27 AM

Hi All,
Are there other issues related to this patch?
If NO could anyone give me LGTM? I need this patch committed to continue with recursive time counters.

efriedma added inline comments.Apr 19 2018, 11:39 AM
lib/Frontend/FrontendTiming.cpp
14 ↗(On Diff #142755)

This should include clang/Frontend/Utils.h .

test/Frontend/ftime-report-template-decl.cpp
2 ↗(On Diff #142755)

What is this test supposed to be testing? If you're just checking that we output the timers, this doesn't need to be so complicated.

We generally prefer to use %clang_cc1 for tests like this.

Please use -emit-llvm instead of -S if you don't actually need assembly.

avt77 added inline comments.Apr 20 2018, 7:17 AM
test/Frontend/ftime-report-template-decl.cpp
2 ↗(On Diff #142755)

You wrote "What is this test supposed to be testing? If you're just checking that we output the timers, this doesn't need to be so complicated."
I'm going to use this test to show new timers soon that's why it looks so complicated at the first glance. At the moment it simply checking the output of the existing counter but later it will show the new counters.

avt77 updated this revision to Diff 143311.Apr 20 2018, 7:19 AM

I fixed issues raised by efriedma.

This revision is now accepted and ready to land.Apr 20 2018, 11:24 AM
This revision was automatically updated to reflect the committed changes.
avt77 reopened this revision.Apr 23 2018, 5:54 AM

It's terrible but my new test was failed again as result of commit of this patch!

/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Frontend/ftime-report-template-decl.cpp:155:11: error: expected string not found in input
CHECK: Code Generation Time
// ^

I don't understand how it's possible. The same problem raised when I committed D43578. Obviously, there is a situation when this test work w/o Code Generation and as result this test is fail because code generation time is zerro. Could anyone help me? How should I change the test? The simplest way is to remove this line but I don't like this idea.

This revision is now accepted and ready to land.Apr 23 2018, 5:54 AM

In any case, when you see a test failing on bots and the fix isn't obvious,
revert first to get the bots back green.

bjope added a subscriber: bjope.Apr 23 2018, 7:43 AM

It's terrible but my new test was failed again as result of commit of this patch!

/b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Frontend/ftime-report-template-decl.cpp:155:11: error: expected string not found in input
CHECK: Code Generation Time
// ^

I don't understand how it's possible. The same problem raised when I committed D43578. Obviously, there is a situation when this test work w/o Code Generation and as result this test is fail because code generation time is zerro. Could anyone help me? How should I change the test? The simplest way is to remove this line but I don't like this idea.

Not sure, but is perhaps the order in which timers are printed not 100% deterministic?
When I run the test "manually" without pipe to FileCheck I get:

===-------------------------------------------------------------------------===
                         Miscellaneous Ungrouped Timers
===-------------------------------------------------------------------------===

   --System Time--   --User+System--   ---Wall Time---  --- Name ---
   0.0040 (100.0%)   0.0040 (100.0%)   0.0021 ( 70.7%)  Code Generation Time
   0.0000 (  0.0%)   0.0000 (  0.0%)   0.0009 ( 29.3%)  LLVM IR Generation Time
   0.0040 (100.0%)   0.0040 (100.0%)   0.0030 (100.0%)  Total

So when I run it without the pipe "Code Generation Time" is printed before "LLVM IR Generation Time".
However, the CHECK:s in the test case are in opposite order.
So I can't really understand why the test passes when I pipe it to FileCheck.

Anyway, if the order isn't deteministic, then a solution could be to use CHECK-DAG instead of CHECK for the checks that may be reordered. For example:

// CHECK: Miscellaneous Ungrouped Timers
// CHECK-DAG: Code Generation Time
// CHECK-DAG: LLVM IR Generation Time
// CHECK: Total
// CHECK: Clang front-end time report
// CHECK: Clang front-end timer
// CHECK: Total

Unless the timers are supposed to be printed in a certain order, then I guess you need to add a sort somewhere in the code.

avt77 added a comment.Apr 23 2018, 8:03 AM

In any case, when you see a test failing on bots and the fix isn't obvious,
revert first to get the bots back green.

I reverted it immediatly.

avt77 added a comment.Apr 23 2018, 8:06 AM

Anyway, if the order isn't deteministic, then a solution could be to use CHECK-DAG instead of CHECK for the checks that may be reordered. For example:

Thank you very much! I'll try to fix it in this way.

bjope added a comment.Apr 23 2018, 8:11 AM

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So that is why it might be sorted differently on different buildbots (or when using pipe etc).

So I think a quick fix is to add -DAG to the checks that can be reorder and submit that fix.

avt77 added a comment.Apr 23 2018, 8:53 AM

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So that is why it might be sorted differently on different buildbots (or when using pipe etc).

So I think a quick fix is to add -DAG to the checks that can be reorder and submit that fix.

I don't see revert as well. But I did the following:

svn merge -c -330571 .

And everything was OK.

buildbots have been failing all day (and were still failing). So I pushed my suggested fix.
I hope that was OK.

buildbots have been failing all day (and were still failing). So I pushed my suggested fix.
I hope that was OK.

Thank you. It seems everything is OK now.

RKSimon closed this revision.Jun 27 2018, 2:33 AM

This was committed at rL330571

Correct me if I'm wrong, but after this change llvm no longer enables the timing of individual passes when -ftime-report is passed? Was that intentional?

Oh, oops, I should have spotted that. No, the point of this was to separate the timing infrastructure, not to change the behavior of -ftime-report.

Should be a one-line change to add "llvm::TimePassesIsEnabled = TimePasses" back to BackendConsumer::BackendConsumer. (Maybe we can add flags to control them separately as a followup.)

Ok I'll add that back.

I'm unclear why the we would want to assign clang's FrontendTimesIsEnabled from inside CodeGenAction. If I'm understanding the intentions here, the goal was to add more timing infrastructure to clang. But if the enabling is tied to CodeGenAction, then doesn't that mean any new clang timers wouldn't work under -fsyntax-only?

Assignment restored in r339281. I'll file a bug to merge to 7.0

avt77 added a comment.Aug 9 2018, 1:28 AM

I'm unclear why the we would want to assign clang's FrontendTimesIsEnabled from inside CodeGenAction. If I'm understanding the intentions here, the goal was to add more timing infrastructure to clang. But if the enabling is tied to CodeGenAction, then doesn't that mean any new clang timers wouldn't work under -fsyntax-only?

Historically, it was done to get compilation times for separate files (to find the longest times). Because of that I did not assume usage of -fsyntax-only. Is it important? Should I move this activity into another place? If YES could you give me a hint where it should be done?

Somewhere around CompilerInvocation::CreateFromArgs is probably appropriate.