I defer to Justin, he knows AST much better than I do.

LGTM

I'm not sure whether it's the right fix.
I would expect the clobbers list of an instruction to be precise - if it says it's clobbered, you can't rely on it being preserved across the call, whatever the circumstances. So maybe the right thing to do would be to filter reserved registers out of the clobber list for these invokes when they're lowered?

In D38676#898099, @aemerson wrote:

A minor nit but LGTM. I don't have an aversion to "dead code" if it's going to be used in the near future, so perhaps to make the patch smaller split it into an initial patch to add VPlanValue.h and then the VPlanBuilder.h as well as the changes to the documentation. Those pieces seem uncontroversial to me, before moving onto the vectorizer. @mkuper does this make sense?

I think the revision summary doesn't fully represent what's going on here at this point. :-)

Thanks, Gil!

Anyway, I agree this isn't the right fix, but my gut feeling is that the right fix is to actually allow the builder to create a bitcast between a pointer and a pointer-sized float.
I don't see anything in the langref that makes the semantics of this undefined. Renato, what kind of semantic issues do you see here?

I agree the vectorizer can "safely" assume this case is ok because we know the original datatype was a pointer anyway and this is part of a strided load, but I feel we'd be opening a can of worms if we start allowing any float<->pointer conversion by default.

For example, a different case would be pointer->float->double->pointer. C has automatic promotions, and some corner cases may slip and create that sequence, which would destroy the bit-pattern and therefore the memory address.

So, if we can do this in this specific case, Manoj's current fix is "better" than moving it up CreateBitOrPointerCast, because we know what the semantics is. Or maybe we just load them as "data" (i32/i64?) and then bitcast safely?

Makes sense?

Should I update the Builder's CreateBitOrPointerCast function to handle float to pointer casts using float -> int and int -> pointer casts? I can do that or I can create a local pass specific casting function to handle float -> ptr + other CreateBitOrPointerCast routines used here.

Why can't you do the bitcast directly inside CreateBitOrPointerCast()? You'll also want to upadte isBitOrNoopPointerCastable(),
Anyway, you probably want a different set of reviewers for that patch - I'm really not the authority on that, and at least Renato seems to have a completely different opinion.

We don't really have a policy on this either way, I think.
I certainly don't have any strong opinions about it.

In D35498#811759, @rengolin wrote:

Setting aside from the fact that no one should ever be casting floating point to pointers, perhaps we shouldn't be even trying to do this transformation here, as I'm not sure this could have a guaranteed semantics in this case, and probably better to just bail the vectorisation?

This looks wrong.
In particular, it would break for something like:

In D35227#805164, @mssimpso wrote:

Hi Michael,

I'm probably missing something obvious here. For external IV uses, we compute the end IV value assuming we're coming from the vector loop. But if we executed the vector loop, shouldn't the SCEV predicate have been true, which would mean that the PSEV was a safe assumption?

Thanks, Wei.
LGTM.

Re overflow - the point is that getOrCreateTripCount() returns, basically, PSE.getBackedgeTakenCount() + 1, and that may overflow, so the "trip count" may end up being 0 if the backedge taken count is 0. I don't think this is outdated, and this is behavior we want to preserve. But this patch should preserve this behavior IIUC. Can you make sure there's a test for this?

I've reverted this in r307334 because it breaks a lot of clang tests with:

LGTM, but please wait for Matt.

In D28907#789851, @RKSimon wrote:

This patch is proving difficult to split into smaller chunks.

@mkuper do you have any suggestions on what can be done to get it done in time for Clang 5.00?

In D34473#789254, @mssimpso wrote:

Michael, do you have any issues with moving to this new interface for the ValueMap?

Makes sense to me, modulo a couple of nits.

Sorry it's taking me so long to get to this.
I've only looked at parts of the code, more forthcoming in the next few days. :-)

This LGTM in terms of being "the right thing to do", but I'm really not sure whether the branch information we have right now is precise enough right now to enable this, especially when using sampling-based fdo.
wmi/dehao, any chance you could check if this causes regressions for us?

LGTM, with a nit.

LGTM

In D32533#738239, @anna wrote:

okay, that explains why we do limit to the target vector register *only* for the store chain in SLP (compile time benefit). However, if we look at the Loop Vectorizer, we consider the maximum vector register size when generating the code in the IR. This also gives a more accurate cost model for LV.

Not considering the physical vector register size is limiting the SLP cost model right? For example, in the target, we would have 4 shuffles instead of a single shuffle.

Adding some target people - I think they ought to care about this more than I do. :-)

Ok, for fast-math, I think this makes sense.
LGTM.

Honestly, I don't feel strongly about this.

In D32451#735969, @twoh wrote:

@mkuper This helps with our internal benchmarks, but I don't have numbers with public benchmarks. What would be the good benchmarks to evaluate the vectorization? Thanks!

When I tried to play with this kind of thing, I got rather inconsistent performance results, because of things like a cold loop within a relatively hot loop.
Do you have any performance numbers for this?

I'm not sure. Do we currently (I mean, without this patch) do anything with FP IVs that violates spec?

Yes, that should have been "trip count - 1", sorry.

The original PR looks fishy to me, but I agree this is a real issue regardless.

Makes sense. LGTM.

What does MSVC do for this? Is MSVC's behavior predicated on /guard:cf? Is it ok to generate empty functions without /guard:cf? Crash on calling an empty function seems like somewhat non-intuitive behavior.
Also, what about thumbv7-windows?

LGTM

LGTM.

This looks ok, in the sense that it doesn't make things any worse, and will catch more cases - but I think it, unfortunately, doesn't really make things better either.
I don't think we can generalize this (because it blows up exponentially), so eventually we'll have to fix the underlying problems with the SLP "tree".

LGTM, Thanks!

LGTM

LGTM

LGTM with minor nit about comment.

LGTM, except for the comments.

LGTM

This makes sense to me, but I don't understand why the Scale == 1 requirement was there to begin with.
+ctopper for another look.

LGTM

I suggest we leave the PMADDUBSW discussion for a separate patch.

LGTM

Ohhh, I think I got it now. Sorry for the noise, let me think about it. :-)

**haven't really looked.

LGTM, but please fix the test before committing.

Sorry for the delay, Matt.

Re the PS - I don't know.
As I wrote above - UnrollLoop() verifies the latch is conditional, but doesn't verify the two edges are actually going to the header and the exit block. But we haven't actually *seen* any issues with this, as far as I know.

Sorry, lost track of this. I'll try to get to this today/tomorrow, unless Elena does first.

In D30757#702406, @mzolotukhin wrote:

Do you know if we already have a utility somewhere that detects this?

Can we require the loop to be in a simplified form and its latch to be exiting? Will it suffice?

In D30757#702345, @mzolotukhin wrote:

Looks good to me, but, as you noticed in the PR - do we want to work with such loops at all? Did you check what happens if we just bail out on such loops?

Michael

ping

LGTM, Thanks!
(If you're not committing the addMetaData fix immediately after this, please put a FIXME above the "if (VF > 1)")

The SLP part LGTM - it's less broken than the rest of the code, and we need to fix SLP itself to fix it (and avoid things like the Container hack...)

Thank you, this will be incredibly helpful. I don't think it quite works, though - see inline.

LGTM

LGTM.

LGTM, modulo the minor issues inline.

I haven't looked at the patch in detail yet, but I don't understand the rationale for putting it in the SLP vectorizer.
If transforming this pattern is a win, it's a win regardless of whether it originated in the SLP vectorizer, or just happened to appear in the IR.

This makes sense. Some minor comments inline.

In D30638#694112, @aaboud wrote:

Notice that we will still need to iterate on the ordered set with a "while(!empty())" because we are removing entries from the Set while iterating on it.

Oy. That's not good. Removal from SetVector (except with pop_back) is linear-time... :-\

Do you need someone to check this in for you?

LGTM.
Also, forgot it before, sorry - please add a test. (No need for a huge test, you can force a small threshold.)