In D62308#1676800, @anemet wrote:

In D62308#1676691, @fhahn wrote:

I'll submit a separate patch for turning ushl -> shl, if the shift is all constant.

... and sshl -> shl?

Add additional test cases and limit this patch to converting cases with appropriate zext/sext.

D67553 adds SimplifyFMAMul. We should probably also have them there. Also, could we make use of multiply by 0.0 here, with the required fast-math flags?

@reames I think this should be good to go in now?

In D67593#1671961, @efriedma wrote:

LGTM

I took a quick glance at what opt is doing, and I see a few differences that likely matter:

1. bugpoint never calls createTargetTransformInfoWrapperPass
2. bugpoint never calls adjustPassManager
3. bugpoint doesn't tweak Builder.LoopVectorize/Builder.SLPVectorize the same way

In D67612#1671640, @asbirlea wrote:

Would something like this work for your purpose?

if (MSSA) {
    auto *D = MSSA->getWalker()->getClobberingMemoryAccess(&I);
    if (MemoryDef *MD = dyn_cast<MemoryDef>(D))
        if (StoreInst *SI = dyn_cast<StoreInst>(MD->getMemoryInst())) 
            if (Optional<SimplifiedAddress> MaybeSimpAddr =
                simplifyNonLoopAddress(SI->getPointerOperand()))
                if (MaybeSimpAddr->Base == AddrBase &&
                    MaybeSimpAddr->Offset == SimplifiedAddrOp)
                    return true;
}

I don't know if you want to return false; on some of those branches or continue with the checks below.

The walker will stop at the first access that may alias the Load. If it's not a Def, it's a Phi and it should give up, similarly if it's a Def but not a Store. Then, only if it's a store and has the conditions you added, the load can be folded/cost is ignored.

In D64884#1661810, @efriedma wrote:

My motivation for this change is to fix a case where we end up with a non-deterministic use-list order after simplifycfg, which I think is caused by this trashing of the use-lists.

I don't see how this could be a problem. Sure, mixing up the order is sort of confusing, but it should get mixed up in a deterministic way.

In D67612#1671613, @asbirlea wrote:

In D67612#1671599, @fhahn wrote:

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.

In D67612#1671617, @fhahn wrote:

In D67612#1671613, @asbirlea wrote:

In D67612#1671599, @fhahn wrote:

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.

It is probably more me not being as familiar with the MemorySSA API. I thought def_chain() would include all memory defs, that could clobber the load, in reverse order. With that in mind, if we hit a non-store or cannot compute the base pointer & offset, we exit. We probably also need another check to make sure we do not miss base pointers that alias. Does that make sense?

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?

Adjust comment and test case.

Updated with @reames' suggestion, thanks!

Thanks Eli, LGTM. This is in line with how we handle verification in many other places.

ping

In D67521#1668482, @asbirlea wrote:

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

Thanks! Could you also add another test where we have a call to lifetime.end, followed by another defining store, followed by a load? This should not get optimized by this patch.

LGTM, thanks! I've just landed D67177, to avoid delays with this patch.

I've put up a patch for SCEVUmin D67177. I plan to land it tomorrow.

Sorry for not being more clear. I meant we need a test where the latch is not exiting, which without this patch will assert and with this patch will return nullptr. So please add a test for something like

Split off simplifications not requiring rounding from SimplifFMulInst as SimplifyFMAFMul.

In D67434#1666337, @reames wrote:

First, please revert the previous patch until this review concludes. We have an active miscompile in tree, and that should be addressed first.

Sure, done.

Ping. Do you think this is helpful on its own? I'll rework the main motivation (D67178), but it still would be useful IMO, e.g. for the test case in D67177.

I've committed an alternative fix rL371595. Thanks Eli! I might submit just the renaming/clarifications to moveLCSSAPhis

LGTM. It would be good if you could prefix the title of the commit with something like [LoopInfo]. In the future, please upload the diff with full context.

Thanks!

In D67367#1663970, @efriedma wrote:

This makes more sense than D67076, I think.

Do the safety checks ensure we never clone an instruction with side-effects? Can we assert that?

Add assertion that moved instructions do not have side-effects, update test checks.

Remove fmul matching.

Is there any chance you could provide the test case that shows the behavior? There might be something we could do in hasDedicatedExits to make it faster for large loops with a large number of exit blocks and successors.

Address review comments, thanks!

I *think* getVectorIntrinsicIDForCall uses TargetLibraryInfo. Looks like we are still including TargetLibraryInfo.h through some other path, but we probably could also get rid of that path.

I've uploaded a new patch D67367. It duplicates and moves the instructions required in the latch. We have to duplicate the instructions and update its users, if we want to support cases where, for example the condition, gets used in the inner loop body as well.

In D62308#1661854, @anemet wrote:

Does this also apply to right shifts?

IMO it is fine to say pragma vectorize(disable) disables the vectorizer completely, including interleaving. @Meinersbur, what do you think? I think it would be good to make that clear in the commit message.

Remove unrelated test-case.

In D67178#1658602, @efriedma wrote:

In terms of general API, I don't think we want to expose "applyLoopGuards"; the SCEV transform proposed here isn't really useful outside of trying to find the minimum or maximum, as far as I can tell. Which min/max expressions we want to form depends on whether we're computing a "max" or a "min". And restricting the API so the point in the CFG we're querying has to be a loop header doesn't seem helpful; other places might care about values after a loop etc.

In terms of the implementation, this composes well, in a sense: it annotates the relevant SCEV expressions with the relevant conditions, then uses the general implementation that ignores control flow. But I'm not sure how it scales for larger SCEV expressions; keeping a map on the side seems like it would have more predictable performance.

ping

In D67076#1659683, @efriedma wrote:

Is there a potential correctness issue here, if some operation that isn't the exit condition or the IV increment somehow ends up in the new latch?

I think the current restrictions avoid any problems. We only allow zext, trunc and cmp instructions between the IV increment and the branch inst. If the final value is used outside the loop, that should be fine. A problem could be occur if any of the moved instructions is part of a reduction. I tried and it seems like we do not recognize reductions with trunc/zext chains at the moment, so we should be covered there. But I guess it would be good to make sure the instructions beside the IV increment are only used in the check for the exit condition.

In D67076#1658545, @efriedma wrote:

I'm not sure how you never hit this issue before. I mean, I can see from the testcase that the compare for the outer loop's branch can end up in the inner loop... but why is it not *always* in the inner loop? What criteria are we using to sink it in some cases? Should we be sinking in all cases?

In D67178#1658197, @efriedma wrote:

Instead of writing a C++ unittest, you should be able to use "opt -analyze -scalar-evolution" to test this.