Good catch.

In D72732#1821201, @fedor.sergeev wrote:

In D72732#1821154, @skatkov wrote:

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

In D70104#1753816, @twoh wrote:

Addressing comments from @skatkov. Thank you for your review!

For X86 asm tests, I just re-ran update_llc_test_checks.py script. I think it should generate "correct" test based on the existing FileCheck patterns?

@manmanren, could you please ensure that your test still checks what it expected to do?

The implementation itself looks good to me.
I worry about tests whether fixes of the tests are expected.
If you have ready answers please let me know otherwise I'll try to dig into them deeply.

Thanks.

The last thing. Would not you like to add a test examine the max-dedicate-exit-iterations functionality?

More tests are added.

In D67207#1660225, @brzycki wrote:

Hello @skatkov , I'm sorry to hear you're dealing with an intermittent crash. If you find a reproducible .ll file you can feed into opt -jump-threading -S < input.ll that crashes I would be interested in examining it. The Deferred usage of DTU causes unexpected issues and it is entirely possible there is still a bug here.

(CC'ing @kuhar and @NutshellySima to keep them informed)

In D67359#1664242, @wmi wrote:

In D67359#1664241, @skatkov wrote:

I wonder whether the problem is in this function or in analysis itself?
If it is a problem of analysis it is better to restrict it but not an utility function.

I feel the problem is in the utility function. Currently the problem is exposed in IVUser pass but the same can happen anywhere else if only the function is called for a huge loop with many exits and with each exit having many predecessors.

I wonder whether the problem is in this function or in analysis itself?
If it is a problem of analysis it is better to restrict it but not an utility function.

Thank you guys, for feedback.

Note: I've got the crash by changing the DomTreeUpdater form Lazy mode to Eager. Even with this fix it will not work because of LVI->eraseBlock(&BB);
It seems the code is written in the way DTU cannot be Eager.

Please provide a quick fix or revert the patch causing the crash due to transforming to incorrect IR.

Please provide a quick fix or revert the patch causing the crash due to transforming to incorrect IR.

Abandon in favor of https://reviews.llvm.org/D64972

Abandon in favor of https://reviews.llvm.org/D64972

In D64972#1607998, @skatkov wrote:

In D64972#1607909, @skatkov wrote:

Philip, please take a look at https://reviews.llvm.org/D65501 which introduces the peeling level as possible solution to handle your comment.

See https://reviews.llvm.org/D65503. It is the same patch but based on D65501.

In D64972#1607909, @skatkov wrote:

Philip, please take a look at https://reviews.llvm.org/D65501 which introduces the peeling level as possible solution to handle your comment.

Philip, please take a look at https://reviews.llvm.org/D65501 which introduces the peeling level as possible solution to handle your comment.

In D64972#1606836, @reames wrote:

Having both the enable flag and the AlreadyPeeled variable is really confusing. Is there a way we could combine them? Maybe replace the boolean with the AlreadyPeeledViaProfiling count or something?

Added the guard to not exceed the UnrollPeelMaxCount limit.

the variable and option name changed.

Test is updated to show what this patch changes.

In D64972#1593826, @reames wrote:

I'm not quite sure this is the right macro approach.

The comment near the code your modifying makes me thing the whole reasoning behind disabling future peeling and unrolling after the first peel may be flawed. It doesn't make sense to "use up" profiling information. Assuming we correctly updated our profiling when doing the transform, the resulting profile for the loop should indicate that it's cold and thus not profitable to further peel/unroll. If that's not happening, maybe there's another issue in play? (I wonder if your other profile bug fix may help here?)

p.s. I'm opened to being convinced that this is a practical answer, even if not an ideal one. Just make the argument. :)

Please, take a look.

Hi Philip, Thank you for review. I'll update a patch.

Abandon in a favor of https://reviews.llvm.org/D64235

Thanks to @davezarzycki who fixed it in
commit 12400b97838d78386920cbda3c9dadb9f8727ce3
Author: David Zarzycki <dave@znu.io>
Date: Mon Jul 15 14:12:35 2019 +0000