LGTM with a suggestion

Sorry for the delay. I think since most of the suggested changes are actually in a follow up patch. LGTM

LGTM, but this patch may add a negligible increment on LLVM's compile time. Make sure you talk to some people that care about the LLVM compile time for their opinions

For some unclear reason rewriteLoopExitValues considers recalculation
after the loop profitable if it has some "soft uses" outside the loop (i.e. any
use other than call and return), even if we have proved that it has a user inside
the loop which we think will not be optimized away.

This also confuse me. Why it must be "soft uses" outside the loop that matter?
What I can guess is that, if there is no use outside, there is *no need* to rewrite the exit value.

LGTM

LGTM in general.

Thanks, I will commit tonight. If you want this patch before that, you could commit it for me.

Fix typo

Address Hal's comment

In D42191#979308, @hfinkel wrote:

In part, it looks like you're looking to modify heuristics that prevent increased spilling around calls. I can imagine that, in general, architectures with lots of registers suffer from less of that, but that's really a statement about the ABI of the call, not the architecture itself.

The original motivation is about hoisting the GEP, in general I want to GVN the gep like:

In D42191#979132, @kparzysz wrote:

There is already a function getNumberOfRegisters. Would it be enough to return some large number from it?

Implement isRegisterRich based on getNumberOfRegisters, and introduce a threshold to tell when the number of registers if big enough to be considered as "register-rich". Also include the usage of the isRegisterRich function in GVNHoist

In D38928#897918, @sanjoy wrote:

LGTM

I think the Loop class has grown quite a bit, and we should pull out some of the utility functions into a separate LoopUtils namespace, but this patch does not need to be blocked on that.

Update comments

Delete the redundant getSCEVAtScope

Update comments

Ping?

Address reviewer comments

Partially address reviewer's comment. This is a WIP patch

Update format

Looks like I need to pass the SCEVExpander from the IndVarSimpilfy pass to coordinate SCEV rewrite, otherwise we will fail test/Transforms/IndVarSimplify/pr20680.ll

Address reviewer's comments

Fix typo

address comments from the reviewer

Update comment

update patch based on the comments

Update comment

Update comment

Update patch to address reviewer's request

In D36800#844026, @grosser wrote:

It's certainly not too late. Until now it seemed to me like a very specific optimization for gfortran. I did not see any such examples before. If we can generalize this, we can certainly try to upstream this. Could you share some of the examples you are seeing?

Sorry this may be too late. Alexandre and me had discussed similar problem (but not exactly the same)

1. maybe we could improve the ArgumentPromotion pass for this
2. this pass seems to be generic, maybe we could move this to LLVM?

In D35858#820816, @regehr wrote:

Could you please quantify the effect that this has on compile time? Time for an optimized -j1 compile of LLVM itself is reasonable. Also it would be nice to know what effect this has on generated code.

Just out of curiosity, are we supposed to change the isl inside polly? what is the relationship between this isl and the "upstream" isl?

Fix typo

Address Tobias's comment

Fix the test case.

Add testcase

I want to also include a test case, if you can point me to the test case related to IslNodeBuilder::preloadUnconditionally, I can add one.
Let me search polly test dir at the same time

fix the last few things before pushing the commit

In D31488#714492, @sanjoy wrote:

Comments inline.

You can also do this same rewrite within SCEV. That is, try to have getSCEV(sdiv instruction) return a SCEVUDivExpr if legal.

This also sounds reasonable and I can also look at that after this.
Just a quick question: are we suppose to use getSCEVAtScope in getSCEV?

address sanjoy 's comment

Add more testcases

Hi Tobias

In D28050#660715, @ahatanak wrote:

I think this patch is an improvement, but Clang::ConstructJob is still one giant function.

Do you have ideas to improve readability of this function or plans to further reduce its size?

Interesting, maybe we need to get this from TargetTransformInfo eventually, because I saw a similar global variable/option in LoadStoreVectorizer as well.

testcase?

This is pushed, phabricator didn't update this patch's status

In D25861#576306, @Meinersbur wrote:

Are you going to review the removal of POLLY_LINK_LIBS in polly and clang separately?