In D37343#875637, @efriedma wrote:

I think this needs more testcases to illustrate the different possibilities here.

Address Krzysztof's comment.

We need to figure out if LSR is doing something reasonable. It's possible that LSR is actually causing the regression, and old jumpthreading is just serving to obscure the loop structure (thus preventing LSR from doing whatever bad thing it's doing).

Address Eli's comment.

In D36404#870092, @hfinkel wrote:

In D36404#869695, @bmakam wrote:

In D36404#869657, @kparzysz wrote:

Have you tried it? Changing jump threading to have early/late versions is trivial, the question is when should the late one run, and if that would help in the first place.

I reverted this change and it recovered the regression. I was going to try to re-run the whole jumpthreading pass after latesimplifycfg to see if it would help, but I don't know if it is a good place because LSR that runs after, will not have the loop in canonical form and loopsimplify will again turn it into irreducible loop. Another place I was thinking was to try before CGP.

If it'll work right before CGP, that's probably best. That way targets can run IR-level passes that want to look at loops before that.

I tried right after latesimplifycfg and it almost recovers the regression. However, running it right before CGP made it even worse for performance of spec2017/perlbench. It seems to be due to some kind of bad interaction with LSR, I haven't digged deeper. Would running right before LSR be reasonable?

kindly ping.

In D36404#869657, @kparzysz wrote:

Have you tried it? Changing jump threading to have early/late versions is trivial, the question is when should the late one run, and if that would help in the first place.

In D36404#835401, @kparzysz wrote:

On our internal benchmarks it's mostly neutral, but the benchmark that motivated it improved by 5.3%. We have some extra code, though, that is supposed to deal with similar cases, so some of the impact may be reduced. By default it keeps the behavior unchanged, so for all other architectures it should have no effect.

Subsumed by D37343

• Added some more checks to allow merging empty cases when we can estimate extra moves are not added.
• Added test cases.
• spec2017/perlbench improves by 4.5%. No other improvements or regressions observed in Spec.

Is the actual value of the PHI operand relevant here? It looks like some of the testcases on r289988 involve constants, which are materialized during isel (and can be substantially more expensive).

I took a stab at this and created D37343. Please take a look.

Thanks for the review.

Move assert closer to AA->alias call.

rebase and fix x86 lit test.

ContainFolderableUsersInLoop -> ContainFoldableUsersInLoop?

Trim the patch and avoid handling stack references for non-value MMOs to prevent LNT test failures.

I have looked into the phi operands closely and I am not convinced if phi operands involving constants have any influence on the profitability of merging empty blocks. I identified empty exit blocks if when merged with their successors improved the performance a bit, yet if another similar empty exit block was merged with the destination block, it sinks the performance. The only difference between them is that the successor block is also empty when the performance regressed.

Thanks for taking a look. Rebased and committed in r310825.

In D35584#829791, @efriedma wrote:

To clarify, the original version of this patch recovered the full 3% regression, but the new version only recovers 0.7%?

Correct.

Is the actual value of the PHI operand relevant here? It looks like some of the testcases on r289988 involve constants, which are materialized during isel (and can be substantially more expensive).

Thanks Eli, this looks interesting. Another observation is that if we increase cgp-freq-ratio-to-skip-merge to 1000 it will recover the full 3% regression. I am trying to reduce the exact basic block which when skipped merging in CGP removes placing copies in hot path and improves performance due to reduced dynamic instruction count and the basic block(s) which when merged eliminate the unnecessary branches and improve the performance due to better branching/i-cache utilization. This might provide an answer to your question about the relevance of PHI operands.

In D36404#834053, @mcrosier wrote:

@bmakam: Isn't this very similar to some of your recent work?

Update to address Eli's comments.

A gentle ping.

Updated patch to address the underlying issue.

The underlying issue is that after loopsimplify creates empty exit blocks, CGP cannot clean up the empty blocks if it happens to be coming from a switch case. I am working on an alternative approach to address this issue.

The CGP changes are here: D35584

Split CGP changes into a separate follow on patch, per Eli's request.

In D35411#813525, @efriedma wrote:

I mean the CodeGenPrepare changes (which are deleting blocks generated by LSR) vs the other changes (which modify transforms that run before LSR).

Minor cleanup. Thanks, Eli.

In D35411#813417, @efriedma wrote:

Hmm...

I took a quick look at the pass pipeline (PassManagerBuilder::populateModulePassManager), and it turns out LateSimplifyCFG is false for the last simplifycfg run. That might be the source of your problem?

In D35411#813349, @efriedma wrote:

SwitchToLookupTable is itself part of LateSimplifyCFG; we should fold empty blocks afterwards, I think?

In D35411#813339, @efriedma wrote:

LateSimplifyCFG cannot catch empty case blocks after switch is lowered, so we need the empty block folding in both places.

"After switch is lowered"? What transform are you talking about?

In D35411#812010, @efriedma wrote:

Why do we need the empty block folding in both LateSimplifyCFG and CGP?

Empty folding in CGP occurs very late after LSR and is also not aggressive because it is not run iteratively. LateSimplifyCFG cannot catch empty case blocks after switch is lowered, so we need the empty block folding in both places.

Update to address Eli's comments.

Minor code clean up, NFCI.

In D35411#811150, @ashutosh.nema wrote:

Thanks Balaram for posting this patch, in general idea looks good to preserve the canonical form of the loops.

Any idea why these benchmark regressed:

spec2006/bzip2:ref -2.18
spec2017/omnetpp:ref -1.2
spec2006/perlbench:ref -1.07

In D35411#809870, @efriedma wrote:

I'm sort of worried this could have unexpected consequences; do you have performance numbers? (LLVM testsuite or SPEC)

I was targeting to unroll a hot loop in spec2017/gcc. In addition to unrolling the hot loop in spec2017/gcc which yielded 2% improvement, I observed a loop interleaved in povray which yielded 6-7% improvement.
Here are the full perf results for SPEC on Falkor with O3 config:

Update latch set and modify a test case. Thanks, Eli.

In D35411#809670, @mcrosier wrote:

Let me see if I can describe the problem and your approach to fixing the issue in my own words.

Currently, JumpThreading and SimplifyCFG avoid threading/merging "empty" loop headers as this would break the canonical form of the loop; the CFG edge being optimized is between the loop header and its successor. Your approach is to also avoid merging the incoming edges (i.e., back edges) to the loop header as well to avoid breaking the canonical form of the loop. Then later in late-SimplifyCFG and CodeGen prepare you more aggressively remove these empty blocks.

Sound about right?

Thanks Chad,
That's right.

Uploading the correct patch. I was being conservative by checking for isConstant only, I can add more cases if you think they are more interesting.

I see the problem now, a part of the patch that addressed Geoff's comments earlier was missing. I will update the correct patch for review shortly.

In D34181#782655, @efriedma wrote:

Any idea what effect this has on performance, if any?

This patch is performance neutral on Falkor/Kryo. This addresses a FIXME that I saw in the passing and attempted to fix as it saves compile time and catch corner cases that are currently not covered.

The changes to the AMDGPU tests need more explanation.

These lit tests were failing due to instruction scheduling changes, so I modified the tests to reflect the new code generation and made the AMDGPU tests more robust.

Address Geoff's comments.

Thanks for the review Chad,
If there is no objection from others, I will commit this change with the fixes for your comments.

Address Chad's comments.

Address Jun's comments.

In D29862#693735, @evstupac wrote:

Regarding other regressions in spec2006.
New method does not guarantee perfect solution. So I think it would be fair to apply it if it generally demonstrate better code.
By generally I mean, that summ of all LSR solution registers (say in a benchmark) become lower.
I can collect such statistic for you arch (please proved me with exact options).
If new method generally select more registers for LSR solutions I'll need to fix this.

Thanks for looking into the regression. I tested D30552 on our AArch64 Kryo target and for spec2006/hmmer it recovered some of the lost performance, however it is still 2% regressed compared to 9% regression previously with lsr-exp-narrow flag on by default.

FWIW, we are seeing a 9% regression in spec2006/hmmer on our AArch64 Kryo target with this flag turned on by default.

Rebase and ping.

Restricted to only irregular symbolic strides such as those found in spec2006/mcf and spec2000/gap. Please take a look.

Do you have any additional comments on this change?

In D24833#552533, @rengolin wrote:

Hi Balaram,

This seems like a well made patch. Correctly enabling the feature, using the pre-fetch when it's profitable and with good tests.

I'll leave the remaining of the reviews and approval to Adam et al, but from my side, the change looks good.