In general, please follow existing style for the file you're editing.

If this got dropped I'm sorry.

I'm OK with this.

This looks fine overall to me.

I don't think this should go in, even off by default as long as it's broken. You can pull the produceSameValue check or fix it, but I don't like having a known-broken pass in tree.

dannyb asked me to take over the review.
Can you fix the issues that filcab identified? I'll see if I can get you an answer about critical edge splitting.

Do you plan on doing something similar for the DSB decode cache issues that occasionally arise?
Specifically this: https://bugs.llvm.org/show_bug.cgi?id=5615

Should we adjust the ratio if it's not specified explicitly and no profile data is available?

Submitted in r310129

Known issues:
needs a test case, and needs an adjustment to the frequency for non-profile functions.

Can you provide a description of what you had to change relative to the rollback, and how you're verifying that the issue that caused the rollback has been fixed?

Looks fine. I would prefer the fallthrough checks be successor checks, but I can accept it either way.

All the checks use unreachable now.

Use cast instead of dyn_cast

Approved with the changes I've marked.

It would be nice to get rid of that negative completely. Not in this patch, but just remove it completely wherever we create CFI Instructions.

This is looking really good. Thanks.

Chandler, After r305934 A test for this isn't possible. We should still commit the fix.

OK. It's looking pretty good overall. I looked a lot closer at the actual CFI Code. Mostly it looks great. I don't think you need AdjustCFAOffset at all. You spend a lot of work maintaining it, but in the one case that it's used, it's just (OutgoingOffset - IncomingOffset). It's only used with blocks that don't contain an offset def.

Mostly looks good. It will be easier to follow when you factor out the NFC sections and rebase.

This is looking pretty good. I'm going to go back over, but most of my initial concerns have been satisfied.

No longer needed because of https://reviews.llvm.org/D34017

In D34017#775713, @danielcdh wrote:

In D34017#775712, @iteratee wrote:

It doesn't have to blow it up exponentially. Can we clone the function to be inlined, re-writing the recursive calls to call the clone, and then inline that? Similar to a worker-wrapper transformation

You mean clone the caller before any inlining, and inline that copy instead of the caller itself in early inlining? Yes we could do that, but that adds unnecessary copy overhead for every caller (which could be large). As llvm does not do recursive inlining anyway, this only solves cases where profile is collected from gcc binary, which should be temporary. So I guess it's not worth the effort to special-handle recursive early inlining.

It doesn't have to blow it up exponentially. Can we clone the function to be inlined, re-writing the recursive calls to call the clone, and then inline that? Similar to a worker-wrapper transformation

This is coming along nicely. I forgot to say last time that I was pleased overall.

Some initial thoughts. I would like to hide the actual CFI algorithms from the existing passes as much as possible.

Tidy up comparisons.

In D33562#766989, @nemanjai wrote:

You talk about a call instruction? Is X2 saved and restored in the called function? Then it's just a CSR and should not be mentioned in the clobber list so no problem with my proposal above.

But it is the caller that saves and restores it, not the callee. The sequence is essentially this (all in the caller of course):

• Save X2 to it's stack slot
• Update X2 prior to the call
• Call the function through a pointer
• Restore X2 immediately after the call

In D33577#766128, @davidxl wrote:

It is basically a choice between a layout (exiting) that has 50% chance of not taking any branches , 25% of taking one branch, and 25% of taking more than one branches vs the new layout that has only 6.25% chance of taking zero branch and 93.75% of taking only one branch.

The existing layout only has 25% chance of taking more than 2 branches -- is it worth sacrificing 43.75% of chances to not take any branches for the improvement for the 25% cases?

Add comments to the change summary about the global heuristic that we're approximating (maybe badly) by calculating the branch factor.

This looks fine to me.

I think you counted wrong. The dynamic taken branch count is the same. But the distribution of the taken branch count is more consistent.

I'm fairly certain these are right, but I wanted to get another set of eyes on them before I committed them.

Committed in rL303084

If either threshold is the only one explicitly set, use that threshold.
Otherwise, if both, or neither are set, use the aggressive threshold at O3

No, I wanted to get agreement before I re-wrote it. I can do it if you'd like to see it before deciding.

Looks fine with the comments I gave addressed.

Now with more Test Case!

Feel free to send me patches that update the other parts of IfConversion

Add splat to the output.

In D33037#750855, @uabelho wrote:

I have on purpose not attacked IfConvertSimple, IfConvertForkedDiamond
and IfConvertDiamond to keep the size of the patch down, and I haven't seen any
test case where they actually go wrong, but I think they suffer from similar
problems since they also use RemoveExtraEdges/analyzeBranch to fix the CFG at
the end.

Ping? Do we know if this vector operation requires the byte splat on power9?

Looks good to me with the changes mentioned.

Change vendor to unknown

Made the aggressive threshold an option.

Note that we should eventually produce the power9 sequence for the non-vector case as well.

Update test to check for the power9 sequence.