Could anybody help to review this last decade's patch?
Thanks a lot!

In D70030#1767393, @huihuiz wrote:

Hey @Carrot , really appreciated your time on it.

For the test case provided, I am pretty sure inner loop and outer loop are in different Loop Chain.

This is correct. I already mentioned it in Nov 15. Thanks for the test case.

ping

The original code doesn't correctly handle the case that the duplicated BB comes from a chain contains multiple BBs. So your patch looks reasonable to me. Could you enhance your comment to emphasize this case. It's better to make the comments of following loop more clearer. The current comments looks similar and confusing.

The explanation looks helpful for understanding the problem.
thanks

More interesting problem is why fillWorkLists is called with a removed BB? From the code around of callers of fillWorkLists, we can see the removed BB is still in MachineFunction or MachineLoop data structure, we should also delete removed BB from there.

Add a new test case.

I tried to build spec2006int, the time changes from

ping^2

ping

@vporpo, it's verified that the eigen test doesn't regress now, actually the inner loop is the same as before.
Thanks

@RKSimon, my command line options are:

Following is a simplified test case that shows the performance regression of eigen. The number of instructions in inner loop is increased from 85 to 93. Hope it can be helpful.

ping

spec 2006 int result is 39.2 vs 39.3, so no change.

In D65303#1661329, @davidxl wrote:

some benchmark numbers?

ping

In D43256#1641056, @hans wrote:

In D43256#1639810, @xbolva00 wrote:

@hans this patch should be reverted from 9.0 I think so rc3 is “fixed”.

I'd rather see the fix land on trunk first (reverting on the branch is also not trivial, there are merge conflicts in several test files). From the discussion at http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20190819/686087.html, it should be ready to go in again.

Will commit this version.

@hjagasia, thank you for the verification.

ping

Patch https://reviews.llvm.org/D65673 for restoring the original layout in plain mode.

In D43256#1609742, @hans wrote:

In D43256#1608459, @Carrot wrote:

In D43256#1607733, @ebrevnov wrote:

I would suggest putting the optimization under an option and disable it by default for now. Once all problems are resolved we can change the default. What do you think?

After restore to original behavior in plain mode, you can use -force-precise-rotation-cost=true to use this more aggressive loop layout.

Is there a patch in progress for restoring to the original behaviour in non-profile mode? It would be nice if we could get this resolved soon.

In D43256#1607733, @ebrevnov wrote:

I would suggest putting the optimization under an option and disable it by default for now. Once all problems are resolved we can change the default. What do you think?

In D43256#1605737, @ebrevnov wrote:

In D43256#1604069, @Carrot wrote:

In D43256#1602841, @hans wrote:

In D43256#1550842, @Carrot wrote:

In D43256#1545519, @samparker wrote:

Do you have any benchmark numbers to show that this is generally profitable? From our downstream testing, it is not clear that this change is beneficial.

We got performance improvement in our internal search benchmark.

How does this transformation impact the benchmark when not using profile data?

In plain mode we also got performance improvement, the speedup is a little smaller than FDO Mode.

I have a general question/comment. By now it's more or less evident that benefit of optimization heavily depends on correctness of profile information. That means in general case there is no way to reason about its effectiveness. Thus I believe it should be turned off if there is no profile.

In D43256#1602841, @hans wrote:

In D43256#1550842, @Carrot wrote:

In D43256#1545519, @samparker wrote:

Do you have any benchmark numbers to show that this is generally profitable? From our downstream testing, it is not clear that this change is beneficial.

We got performance improvement in our internal search benchmark.

How does this transformation impact the benchmark when not using profile data?

Benchmark is running, will report the result once it is finished.

isnan() is lowered to fcmp uno, in this case the taken probability may be higher, but still much smaller than ordered result, and the usage of isnan() is already very rare, most of uno comparisons are generated from different math library functions.
On the other side I guess most of the taken uno comparisons in a correct program comes from explicitly isnan() call.

Comment change.

@ebrevnov, it's better to provide a reproducer. Otherwise I can't analyze the problem that impacts your code. Four is not a big number.

ebrevnov, with following patch the test case can be layout correctly. Can you try it with your actual code?

Thanks for the test case, I can reproduce it now.

Evgeniy, could you try to build your code with FDO? The layout code is based on profile information, if that is not available, the static estimated profile information is used. Since the loaded values are not NaN, there should be no branch from loop header to latch, but the estimated profile gives it a non trivial value, so the latch is moved before header, and one taken branch is reduced in the NaN path. I think the static profile estimation can be enhanced to treat a floating point number as not a NaN, or as a NaN only with a very small possibility.

In D64376#1581825, @xbolva00 wrote:

What about CTMark? SPEC?

Add comments.

Add a simple CFG example to show the inefficient tail duplication.

In D43256#1545519, @samparker wrote:

Do you have any benchmark numbers to show that this is generally profitable? From our downstream testing, it is not clear that this change is beneficial.

Some analysis of the test case changes.

In D62430#1523424, @kbarton wrote:

Hi Carrot,
I agree with this change, conceptually. Have you done any performance measurements to see what the impact is?

Changed the edge frequency distribution, so I can avoid floating point square root computation.

Update the patch to the current code base, also make two improvements:

Precisely compute the fallthrough frequency is very helpful in layout of loop header and exit BB. It is included in my next patch.

Just FYI.