LGTM

Instead of adding a new flag, would it be possible to make this part of the machine verifier? (-verify-machineinstrs)

LGTM with a small refactoring. I would wait until tomorrow for other reviewers to react, if not you can go ahead and commit this.

In D45948#1084799, @aheejin wrote:

There are cases in which i32 is used to signify a constant type:
Example 1
Example 2
And there are many more. So changing the category of i32 to an identifier will cause all these tests to fail.

I see the problem. The solution feels a little hacky though. Would it make more sense to get rid of MIToken::IntegerType and lex i32 to identifier?

In D46278#1083284, @efriedma wrote:

Do you have any idea why this is happening? I would expect that normally, instruction selection won't insert a useless csel. (If we're missing some fold before isel, fixing that could have a larger benefit overall.)

Sorry for the wait, this looks very good! Did you see any compile time impact on this?

LGTM with a small comment. Thanks!

Add comment for int Slot;.

Fix a few tests.

• Rename di-* to debug-info-* for fixedStack: and stack:
• Add comments to test to explain what they're testing

LGTM, thanks!

LGTM, thanks!

I think @gberry's solution is actually the correct fix. I don't really see why we shouldn't use empty stack slots if we can.

In D42848#1073681, @violetav wrote:

@thegameg Hm, that's strange. They pass on my side. What are the failures that you're having?

I have some tests failing on my side:

Thanks for working on this again!

Thanks Jonas!

local_unnamed_addr is probably not needed either.

Sorry for not looking thoroughly at the test in the previous patch.

IIRC, there was a test case in the original patch but I can't see it in the landed commit. Any idea what happened?

This seems to break the Machine Verifier. I filed PR37153. Do you mind taking a look please? Thanks!

Thanks!

Thanks for working on this! This looks good to me. Adding Tim for the AArch64LDST part.

Thanks. LGTM + one more comment.

LGTM, thanks!

Thanks! Your suggestion is much more readable.

In D44958#1059982, @junbuml wrote:

I think I saw the case where LiveRegUnits is used to find a scratch reg. It seems possible to use it here as well. Let me take a little closer look at it. I think it should be a separate NFC patch.

Thanks for this! If it's possible, could you also write a MIR test for this?

Ping?

Thanks for working on this.

Sorry for the delay.

LGTM, thanks!

LGTM, Thank you!

In D45189#1055625, @gberry wrote:

Another thought, would it be worthwhile to add a serialized MIR function property for this usesRedZone flag so you could write MIR tests for this instead?

This LGTM. If you (or anyone else) can come up with some kind of test it wouldn't hurt, but I remember your test from the previous patch was all right.

The shrink-wrap pass would already assert here. Could you add the property to ShrinkWrap as well?

LGTM, thanks!

Sorry for breaking this. There is already a patch here: https://reviews.llvm.org/D44871.

LGTM. Thanks for fixing this the right way, it looks much better now!

Thanks! I remember wanting to do this, I must have missed it, sorry.

In D41463#1043179, @junbuml wrote:

In my test with Os, I can see -0.6% in spec2000/gcc, but I couldn't see any change in the hot function (propagate_block), which takes just 11% in profile though. As far as I check in my environment, 0.6% seems reasonable performance swing. I'm not sure if your regression on 176.gcc could be explained as swing or not. Please let me know you see any negative impact from this.

Added @craig.topper for the X86FrameLowering::mergeSPUpdates changes.

Thanks for this @junbuml! I did some runs on arm64 and I see a regression on 176.gcc, in average of 1.5%.

Thanks for the review Justin!

Handle (hopefully) all the different cases.