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

Add test

In D63148#1538399, @lebedev.ri wrote:

Missing a test

Hi @john.brawn , here is the fix, could you please look to it: https://reviews.llvm.org/D63148

The issue actually is not with this instruction, but it's related to exception handling. Hoisted instruction is inserted before getFirstTerminator(), but there could be EH_LABEL's which are not terminators, but could change control flow.

In D56772#1537772, @john.brawn wrote:

This is causing incorrect code generation for this piece of IR:

target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64-arm-none-eabi"

@var = hidden local_unnamed_addr global i32 0, align 4
@_ZTIi = external dso_local constant i8*
declare dso_local void @_Z2fnv() local_unnamed_addr #1
declare dso_local i32 @__gxx_personality_v0(...)
declare i32 @llvm.eh.typeid.for(i8*) #2
declare dso_local i8* @__cxa_begin_catch(i8*) local_unnamed_addr
declare dso_local void @__cxa_end_catch() local_unnamed_addr

define hidden i32 @_Z7examplev() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
entry:
  invoke void @_Z2fnv()
          to label %try.cont unwind label %lpad

lpad:                                             ; preds = %entry
  %0 = landingpad { i8*, i32 }
          catch i8* bitcast (i8** @_ZTIi to i8*)
          catch i8* null
  %1 = extractvalue { i8*, i32 } %0, 0
  %2 = extractvalue { i8*, i32 } %0, 1
  %3 = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*))
  %matches = icmp eq i32 %2, %3
  %4 = tail call i8* @__cxa_begin_catch(i8* %1)
  %5 = load i32, i32* @var, align 4
  br i1 %matches, label %catch1, label %catch

catch1:                                           ; preds = %lpad
  %or3 = or i32 %5, 4
  store i32 %or3, i32* @var, align 4
  tail call void @__cxa_end_catch()
  br label %try.cont

try.cont:                                         ; preds = %entry, %catch1, %catch
  %6 = load i32, i32* @var, align 4
  ret i32 %6

catch:                                            ; preds = %lpad
  %or = or i32 %5, 8
  store i32 %or, i32* @var, align 4
  tail call void @__cxa_end_catch()
  br label %try.cont
}

As part of the accesses to var an ADRP instruction is being generated and MachineCSE is hoisting it to the entry block, but it gets hoisted to after the call to fn so when we catch an exception the ADRP hasn't been executed so the loads and stores use an undefined base register.

Closed by commit https://reviews.llvm.org/rL362901

In D61914#1534508, @plotfi wrote:

cfe/trunk/test/Driver/check-time-trace.cpp appears to fail on Darwin. Did you mean to pass the target explicitly ?

In D61914#1512021, @aganea wrote:

Could you please move the test to a more approriate location? (ie. clang/trunk/test/Driver/)

Updated