It has been reverted.

@aeubanks, can you extract a test case from your failure?
It will be very helpful for me to debug the problem.

Then it should be an unknown bug in MBP.

As jmorse has pointed out, the function layout is incorrect at the assertion time.

@dmajor, without a test case it is difficult to point out what's wrong since there is no obvious relation between this patch and VarLocBasedImpl.cpp. I will try my best to do some simple analysis/guess:

For the attached test case, it can cause bad layout, block .problem is placed under .cold. With this patch .problem is optimally placed after .pred.

It can be triggered by a test case in my next basic block duplication patch, because newly created basic block can cause MBFI.getBlockProfileCount crash.

LGTM

Please also update your patch description. Otherwise it looks good to me.

I think your test case is a perfect candidate for this optimization. It should not be skipped.

Will commit this version.

In D83265#2144298, @davidxl wrote:

A few things:

1. can this be extended to the TailDup pass too?

This patch only changes the cost function, it doesn't change tail dup algorithms, especially the partial duplication is not implemented in TailDup pass.

@fhahn, could you help to take a look at https://bugs.llvm.org/show_bug.cgi?id=46335? It may be caused by this patch.

ping

In D80943#2067284, @efriedma wrote:

See https://reviews.llvm.org/rL51019 for the original commit.

The benefits of matching loadi32 like this are:

1. mov is one byte shorter than movzx
2. We can fold the load into a subsequent 32-bit instruction in some cases.

And the downside, of course, is that if we're unlucky and create a hazard, there's a performance hit.

In D68414#2035503, @efriedma wrote:

I'm happy with the general approach here; just some questions about the details.

Also, can you gather numbers about how frequently this triggers over some testsuite like the LLVM testsuite or SPEC? I'd like some idea about how large the impact of this is going to be.

This version splits overlapped AllocaSlices like

Thank you for the example.

In D68414#1978709, @efriedma wrote:

Your reply doesn't really address the most important point. The entire premise of your extractTypeFields is that the type of an alloca is a reliable guide to the way code will access that alloca. That simply is not true: clang cannot generate appropriate LLVM types in some cases. So you'll end up with weird behaviors in many cases.

LGTM.
Thanks for the fixing.

The most complex part of this patch is avoiding "blindly rewriting", and most of the new data structures are used for this purpose.

ping

I took a look at the first failed test case amdgpu-hip-implicit-kernarg.cu, it contains an empty kernel function, it should not trigger any optimization code.

This patch doesn't do any register level changes, so it looks unlike the root cause of the failure, maybe some unrelated bug was uncovered by this patch.

ping

Thanks, will commit this version.

ping

In D73387#1839878, @xbolva00 wrote:

New improvement looks cool, any data for clang itself (large binary too)?

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.