@rsmith , Can you reproduce the error my case give?

This patch has caused a regression on PowerPC(target powerpc64le-unknown-linux-gnu).
Below is the test case:
cat test.cpp

struct a1{};
struct a2{};
struct a3{};
struct a4{};

Modified the patch to follow Nemanjai's comments.

In D71983#1799470, @hfinkel wrote:

Do you expect any effect at all from doing this? These are all scheduling barriers?

In D71983#1799470, @hfinkel wrote:

Do you expect any effect at all from doing this? These are all scheduling barriers?

I have tested the spec performance for this patch, there is no degression.

I have run the performance for this patch, there is no regression.

I have done the performance test for this patch, there is no degression.

Set the hasSideEffect flag for on instructions instead for format.

In D71390#1788249, @nemanjai wrote:

Something doesn't match up here. The description/title say that you are changing the flag for both MFLR/MTLR. But in the code I only see the change for MTLR. Why is that?
Also, the context is missing from this patch so it cannot be reviewed properly, please fix that.

Add the context.

Fix the empty line.

In D71390#1780987, @lkail wrote:

Seems currently we haven't modeled mtlr correctly. In PPCFrameLowering

if (MustSaveLR)
  BuildMI(MBB, StackUpdateLoc, dl, MTLRInst).addReg(ScratchReg);

We might lack a implicit-def of $lr here. As a result, we might get wrong code if set hasSideEffect = 0 for mtlr. What do you think?

Set the hasSideEffects in VX1_RT5_RA5_VB5 .

In D71391#1781086, @Jim wrote:

It looks like not only this 6 instructions have to set hasSideEffects to 0.

Modify the patch.

In D69437#1728336, @dmgreen wrote:

Hmm. That's interesting. I'm guessing the flags you use between base and peak are quite different? The only consistent results was exchange2_r, which was getting better! How noisy are these results? What kind of a confidence interval do you have?

It may be simpler to make this AArch64 only instead, same as the X86 backend previously had. We know that there it fixes the issue with repeatedly spilling registers, and the performance is otherwise not largely effected.

Use the script to update the test cases.

Update the test cases using the tool update_llc_test_checks.py.

In D69437#1721706, @jsji wrote:

Can you add tests for other targets as well? eg: PowerPC, RISC-V?
And maybe pre-commit the testcases, so that it is easier to see what the diff .

@ZhangKang Can you help to do size and performance evaluation on SPEC2017 on PowerPC. Thanks.

In D66576#1714164, @xbolva00 wrote:

I can.. but next week.

Maybe @Carrot @evandro @fhahn could check it too.

In D66576#1705646, @xbolva00 wrote:

Do you have numbers also for x86?

In D66576#1705646, @xbolva00 wrote:

Do you have numbers also for x86?

In D66576#1705534, @lebedev.ri wrote:

In D66576#1660216, @steven.zhang wrote:

.AMDGPU also override the getCSRFirstUseCost() but your patch didn't catch that. And would you please post some improve number for powerpc of this patch ?

Not done; would be good to have some perf numbers here, for ppc and x86

Fix the error for jump table.

In D59403#1639298, @jsji wrote:

Looks like you did not commit the version (Diff 190782) that was accepted!

So introduced duplicate documents in commits (Diff 192788) .
Thanks @davezarzycki for noticing this and fix it in https://reviews.llvm.org/rL369496.

@ZhangKang Please pay more attention next time. Thanks.

The patch [MBP] Disable aggressive loop rotate in plain mode has modified the test case, so I update the test case to sync the test case.

@efriedma , I have updated the patch, do you have any comments?

Add the comment about post-dominator tree.

In D63972#1612791, @efriedma wrote:

Can you remove the dead MachineDominatorTree *MDT; declaration?

the function MachineBlockPlacement::runOnMachineFunction will do some clean work and will never use the MachinePostDominatorTree info, and this pass don't preserve the MachinePostDominatorTree

Please add this explanation as an explicit comment in the code.

Update the MachineLoopInfo.

In D63972#1608824, @efriedma wrote:

Okay, delaying some of the work to sense, since you can't really modify FunctionChain while you're iterating over it.

Any response to my other comment?

Looking over this patch again, some of the other work involved in updating various data structures isn't complete here; the MachineDominatorTree/MachinePostDominatorTree isn't updated, MachineLoopInfo isn't updated.

Use the for range loop.

Move the clean EmptyBB work out of the for loop.

In D63972#1606967, @efriedma wrote:

Here, if I use F->erase(TBB), the memory leak error is still existed.

What exactly is leaking? (If you're calling erase(), it isn't the MBB itself.)

Of course, that isn't a substitute for calling FunctionChain.remove etc.

Looking over this patch again, some of the other work involved in updating various data structures isn't complete here; the MachineDominatorTree/MachinePostDominatorTree isn't updated, MachineLoopInfo isn't updated.

In D63972#1605006, @efriedma wrote:

You probably want F->erase(TBB), which both removes TBB from the list of blocks in the function, and deallocates TBB. I guess the empty block with no predecessors doesn't really matter much, in the long run, but easier to understand if the transform cleans up after itself properly.

The old patch has the memory leak error.
The new patch fix the memory leak error by using:

FunctionChain.remove(TBB);
BlockToChain.erase(TBB);

instead

F->remove(TBB);