Update iterator var names.

Nit: rename optional argument.
I'd like to check this in, in an effort to clear my patch queue. Please let me know if you have any comments.

Thank you for the review!

Address comments.

lgtm.

Maybe elaborate in the patch description what determine when and how we will unroll loops. means?
e.g.:
"The default before and after this patch is for LoopUnroll to be enabled, and for it to use a cost model to determine whether to unroll the loop (OnlyWhenForced = false). Before this patch, disabling loop unroll would not run the LoopUnroll pass. After this patch, the LoopUnroll pass is being run, but it restricts unrolling to only the loops marked by a pragma (OnlyWhenForced = true).
In addition, this patch disables the UnrollAndJam pass when disabling unrolling."

Thank you for this!
LGTM.

I'm seeing the attached test pass at ToT now.
I may have resolved it in rL373380, which was failing to remove trivial Phis when inserting a backedge block.
Please let me know if you are still seeing issues!

This already landed in D62981 .

Add testcase.

Thanks for the cleanup! A couple of small comments inline.

lgtm.

Gentle ping on this.

Added const.

Perhaps worth mentioning that the walker also does phi translation when walking defs upward.

MSSA should have already done this verification. The walker will skip over stores that do not clobber the load or other MemoryAccess-es that do not interfere with the load.
So if there is a Store with the same pointer and different offset, the aliasing result for the two should be that they don't alias, and the walker will continue until it finds an access that does clobber the load.

Address comments.

Would something like this work for your purpose?

In D67612#1671599, @fhahn wrote:

In D67612#1671530, @asbirlea wrote:

Here are a couple of answers/discussion points:

First of, could you please elaborate a bit more on what you're trying to check for? Is it stores before the loop, after the loop or both?

The idea here is to check stores before the loop and use the store to determine the loaded value in a specific loop iteration. We only do that if we can determine a base pointer + constant offset for a certain loop iteration.

This is the part I do not understand. It looks like the LoadInst is considered "free" and not marked towards the cost of the unroll if there is a store with the same base pointer + offset.
Is that always the case? If there is an interfering MemoryAccess between the two (the walker's answer), then the LoadInst is not free.
I'm not that familiar with the code here, so feel free to correct me if I misunderstood.

Here are a couple of answers/discussion points:

Make the update in MemorySSA.

Why does an instruction which doesn't read or write memory have an associated MemorySSA memory access? Do we assume everything accesses memory if basicaa is disabled?

Except for a few cases, we rely on getModRefInfo to add MemoryAccesses to Instructions, so, yes, without basicaa, we're creating accesses where we shouldn't.

Would it make sense to fix that, instead of adding checks which always fail normally?

Yes, that would make a lot of sense! I was thinking of adding this as as alternative in the patch description so I'm very happy you suggested it.

lgtm with a nit: clang-format (some lines seem longer than 80)

Thank you for the suggestion and review.

Address comments.

Update based on comments.