Yes, I agree. LGTM.

I believe it's better solution, LGTM.

In D68161#1686632, @lebedev.ri wrote:

Oh nice!

I like this.

One more related question - there are two "Frontend" sections, second one appears to consist only of "CodeGen Function"s.
Is that correct? Should they both be in an actual front-end section, and should they be renamed to, i dunno,
"Sema" and "Codegen"? I'm not sure about this, just raising a question.

In D62067#1672835, @lebedev.ri wrote:

With new-pm soon being the default, i'm curious what's the plan with -ftime-trace?
Will it simply turn into a pumpkin? Is there an upcoming patch (along the lines of D62067#1511128)?

Could you please give LGTM?

Hi @nathanchance, @mgehre, fixed here: https://reviews.llvm.org/D66873, please, look at it to commit.

In D63325#1645892, @mgehre wrote:

I'm seeing the same issue Not all CodeGen sections are inside any Frontend section! with python 3.7.1. Json:

check-time-trace-sections.json15 KBDownload

In D63325#1643023, @nathanchance wrote:

Done, thanks for looking into this!

check-time-trace-sections.json14 KBDownload

Hi @nathanchance , I could not reproduce your failure using the same revision and build-llvm script. Could you please attach your json report located at /home/nathan/cbl/git/tc-build/build/llvm/stage1/tools/clang/test/Driver/Output/check-time-trace-sections.json?

Ping!

Rename

In D66132#1626902, @lebedev.ri wrote:

After reading through https://bugs.llvm.org/show_bug.cgi?id=42405 i'm not confident that this is the correct solution.
It reads to me as-if this speculative execution in PRE being done with no proper legality checks.
In particular, i'd think you want to check a white-list in MachineCSE::isPRECandidate(),
much like llvm::isSafeToSpeculativelyExecute() is for middle-end.

Unless i'm really looking in the wrong place, there is no such thing here?

In D66132#1626811, @lebedev.ri wrote:

I feel like this is lacking words.
What problem is this addressing?
Is it correctness-one or performance?
The current hook feels a little bit weird.
Does it mean to completely ban speculative execution?
If then there is a lot more places where it should be used, and not using it there may be surprising.
Or does it only mean to ban PRE? But then, what was the original problem?
Or should it be per-MI hook?
etc

Update comment

Hi @LuoYuanke, I've benchmarked the effect of the revertion my and @lkail patches.
The benchmark showed some increase of the exec_time:

~/llvm/test-suite/utils/compare.py  --filter-short -m exec_time ~/llvm/test-suite-results/results_rel_base.json ~/llvm/test-suite-results/results_rel_base2.json vs ~/llvm/test-suite-results/results_rel_exp.json  ~/llvm/test-suite-results/results_rel_exp2.json

Ping!

Ping!

Hi @lebedev.ri, could you please lgtm this or elaborate on possible issue with this?

Remove default repeat

Hi all! Could it be accepted or reviewed more?

Update

In D56772#1596814, @LuoYuanke wrote:

@anton-afanasyev
Hi,
Do you have any performance data for the patch? I'd like to know what benchmark has performance gain with your patch. https://reviews.llvm.org/D64394 fixed perlbench regression, but I wonder what the performance gain do we achieve with the 2 patch?

Btw, this fix (https://reviews.llvm.org/D64394) was commited recently.

In D56772#1592951, @LuoYuanke wrote:

@anton-afanasyev
Hi,
Did you look into the SPEC cpu2017/500.perlbench_r issue? There is some significant performance drop on X86 with the patch. I ask you to revert the patch first, and when the SPEC2017 regression is fixed, we can submit the patch again. How do you think?

Hi all! I've figured out that my current implementation of PRE is incorrect and it needs to be either corrected, restricted or reverted.
The issue is that current PRE can actually hoist instruction executing it speculatively for some CFG paths which potentially leads to perf degradation. However I believe that current heuristic PRE is rather useful if it is restricted enough. Good restriction (apart from this revision) is implemented by @lkail here: https://reviews.llvm.org/D64394, it is based on basic block frequencies comparision.

Btw, this change breaks multiple (more than two) hoisting to common dominator. I've tested this patch for the original test case taken from here: https://bugs.llvm.org/show_bug.cgi?id=38917. There are several comparisons giving 96 > 40 + 10, 96 > 29 + 10, 96 > 18 + 10 (so no hoisting at all), meanwhile 96 < 97 = 40 + 29 + 18 + 10.
However I do not see easy solution for this issue.

Ping!

In D63934#1584574, @aymanmus wrote:

Thanks Anton.
Can you please keep us updated with any progress in this regard?

In D63934#1575308, @aymanmus wrote:

Just to make sure I understand the change correctly:
This can still hoist instructions outside of their BB and execute them speculatively even if you can't prove that all possible paths execute the instruction, am I right?

In D63860#1574717, @piotr wrote:

In D63860#1572882, @anton-afanasyev wrote:

Hi @piotr, have you tried regression benchmarking with this patch? For instance, test suite.

Hi Anton, Yes, I have just run regression testing on the test-suite and the tests pass ("Expected Passes : 917").

Add small fix

Hi @piotr, have you tried regression benchmarking with this patch? For instance, test suite.

Ping!
This patch fixes https://llvm.org/pr42405 implicitly.

I've upload json-file and printscreen of its visualization (used https://speedscope.app). This is an example of how this patch works, making two Frontend sections.

Added test

In D63934#1562377, @lebedev.ri wrote:

(is this missing a test?)

Is it ok now? I doubt that main code refactoring is a good way when adding support timer code. So ended with more robust solution, though it leads to two Frontend sections.

The bug is reported by @aymanmus here: https://llvm.org/pr42405

In rL362901#665441, @aymanmus wrote:

Hi Anton,

Attached is an mir test for a reproducer to a bug that we encountered that appears to be an incorrect behavior of MachineCSE pass after this change was committed.
Can you please take a look ASAP?

You can run it using the following command:
llc -mtriple=aarch64-none-linux-gnu -run-pass=machine-cse -asm-verbose=1 -verify-machineinstrs reproducer.mir -o after-pass
You can see that the divide instruction was pulled out of the if-if-else and it can divide by 0 when the original code doesn't allow this.

Thanks,
Ayman Musa

reproducer.mir4 KBDownload

Changed comment

In rL362901#663831, @asbirlea wrote:

@anton-afanasyev: FYI, we're still seeing this and it presents itself as a miscompile.
We're still working on getting a test case but it's proving to be very difficult.

Also, first "Frontend" section contains "ParseTemplate" and "PerformPendingInstantiations" sections.

In D63325#1553458, @lebedev.ri wrote:

Hm, i started writing previous comment before you posted your last comment, so i didn't see the last update.
This looks less intrusive, yes, but two observations:

1. You now have two "Frontend" sections - first one being for lexing time
2. That lexing section is not within the "Frontend" section, even though it is for sure part of frontend of the compiler. Like i said, i'm not sure what the right solution here is.

Hi @lebedev.ri, I've turned back to the solution with one TimeTraceScope for "Frontend" inside BackendConsumer::HandleTranslationUnit(), since it looks more robust. This admits several (two for now) "Frontend" sections, but I see no problem with this. Other solutions have to rely on BackendConsumer and ASTConsumer calling correct relations, which are not guaranteed (though it is looking correct for now).

Updated, changed test

In D63325#1549316, @lebedev.ri wrote:

Hmm, i'm only now noticing how fraglie this looks.
How do we know that clang::ParseAST will only ever be called with BackendConsumer ASTConsumer?
How do we know that BackendConsumer will only ever be ASTConsumer from clang::ParseAST?

Updated

Updated

Updated

Small fix

In rL362901#661288, @asbirlea wrote:

@anton-afanasyev:
The test failures occur in Halide generated code, with the HVX backend. They are not public.
r363164 does not fix the tests; it does change an erroneous output to another erroneous one.

@asbirlea Btw, this fix was commited for the issue noted here: https://reviews.llvm.org/D56772#1537772

In rL362901#661260, @asbirlea wrote:

FYI, we're seeing test failures due to this revision. Currently trying to isolate a testcase, but it may be challenging to minimize if the failures are due to a miscompile.

Last issue fixed by this revision: https://reviews.llvm.org/D63148

Update description