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.

Will check in this version.

ping

Thanks a lot!

A bug https://bugs.llvm.org/show_bug.cgi?id=41129 is filed for the regression.
Thanks a lot for the investigation.
Please let me know if more information is required.

This patch causes 5% regression of one of our eigen benchmarks on Haswell.

ping

Any other comments?

Add comment to explain the case when the inline asm occurs between branch block and dest block.

In D57718#1402795, @nemanjai wrote:

I can't help but feel like this patch adds further complication to an already excessively complicated function. To put things in perspective - this file is currently 282 lines and 210 of those lines are in a single function. Frankly, I think refactoring this into a few conceptual sections implemented in separate functions would go a long way.

Seems to me that conceptually this pass does the following:

• Renumber blocks
• Compute the size of each block
• Identify PC-relative branches and compute the branch distance
• Convert PC-relative branches that branch too far into the "long branch sequence" (i.e. invert branch condition, convert to bc+8, b)
  
  I understand if you don't want to do this refactoring and want to proceed with this patch as-is to solve the immediate problem. However, I thought I would bring this up since it seems like refactoring this would go a long way to making this readable.

Also handle the backward branch.

Add a test case.

In D57718#1396611, @sdardis wrote:

In D57718#1393615, @Carrot wrote:

In D57718#1393504, @jsji wrote:

Any testcases that can show the problem and test the fix? Thanks.

It can only be triggered by a very large (>32KB) function body because the range of conditional branch is +/- 32KB.

In our case, the large function body is caused by aggressive thinlto guided inlining.

So no small test case can demonstrate the problem :(

You can use the assembly directive '.space' to create arbitrary sized basic blocks. See test/CodeGen/RISCV/branch-relaxation.ll for examples.

In D57718#1394924, @jsji wrote:

I think we should be able to come up with a smaller test case that demonstrate the problem here?
No necessary causing run time problem due to wrong branch,
but causing smaller calculated branch offset, and we can check that by adding a few dbgs() to check calculated branch offset in code?

Because of the code at line 137

In D57718#1393504, @jsji wrote:

Any testcases that can show the problem and test the fix? Thanks.

ping

Thanks for the explanation.
LGTM

I couldn't understand the problem after reading the description.

In D57067#1369587, @davidxl wrote:

lgtm.

Do you have any benchmarking numbers?

This patch doesn't correctly handle isFast(). By default isRecurrenceInstr() should check I->isFast(), but for this pattern I is Select, isFast() doesn't apply to it, it should be checked against FAdd/FMul inside isConditionalRdxPattern().

In D49512#1190373, @qcolombet wrote:

To be more precise, the latest promotion should be correct, but if we want the full information of multiple extension, the current patch is I believe wrong.
E.g., zext (sext i8 to i16) to i32, doesn't give you the same warranties on the bit pattern than what zext i8 to i32 gives, whereas we won't be able to see that.

In D49512#1190372, @qcolombet wrote:

Shouldn't we keep only the latest type and kind of extension?

friendly ping.

ping

In D49512#1167869, @grandinj wrote:

from someone who is just an observer: What happens when we see IsSExt types

A
B
A

?
Then the last one will re-insert A, which is probably not what you want.

In D47346#1111861, @chandlerc wrote:

We have an MI combine pass IIRC -- why not put this there? (Genuine question, haven't looked at these enough to know if that doesn't work...)