We shouldn't make this either/or. Ability to runtime check unit-stride is good, and ability to use gather/scatter is also good. Depending on the target, I see the following situations

1. don't vectorize the loop ---- unit-strided code alone isn't profitable or loop itself is profitable but not good enough to cover runtime check cost.
2. vectorize with runtime check, with scalar code to fall back ---- when gather/scatter code is deemed not profitable.
3. vectorize with runtime check with gather/scatter code to fall back
4. vectorize with gather/scatter

LGTM. Thanks!

This patch looks good to me. I agree it's nicer if we could fix it during widenInstruction, but let's leave that fix to the VPlan centric code generation, where we expect to (eventually) have the necessary context.

In D69563#1748967, @dantrushin wrote:

@hsaito: What do you think?

Are we essentially saying that any reassociation can't preserve NSW/NUW flags?

We have observed some performance regressions, presumably because the vectorized code started to kick-in on short estimated trip count loops (as opposed to skipping vector code and execute scalar code). We'll try following up with cost model tuning. I'm not too surprised if others also hit a similar issue. Overall, though, that's the right direction to head to.

In D57504#1724196, @cameron.mcinally wrote:

+1 on what Simon said.

In D67948#1722120, @hsaito wrote:

In D67948#1722099, @hsaito wrote:

In D67948#1717671, @ebrevnov wrote:

Updated comment as agreed.

Got it. First time committing through git (SVN is read-only now). Expecting some learning curve. If you'd like this in sooner, you might want to ask someone familiar with the process. Will see if I hit any issues.

Needed to have done this process before SVN went readonly. Please ask @reames to commit. He must be better prepared. Sorry about that.
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access-to-the-github-repository

In D67948#1722099, @hsaito wrote:

In D67948#1717671, @ebrevnov wrote:

Updated comment as agreed.

Got it. First time committing through git (SVN is read-only now). Expecting some learning curve. If you'd like this in sooner, you might want to ask someone familiar with the process. Will see if I hit any issues.

In D67948#1717671, @ebrevnov wrote:

Updated comment as agreed.

In D69088#1715210, @Meinersbur wrote:

I'd rather just enable them with a command-line switch, such as -fexperimental-transform.

In D67948#1712213, @ebrevnov wrote:

In D67948#1711380, @hsaito wrote:

In D67948#1710615, @ebrevnov wrote:

@hsaito, Please commit if you find this version acceptable.

@ebrevnov, I recommend you visit http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and obtain your own commit access. Your committed and under review patches deserve it.
Thank you very much for your contribution. Let me know if you still would like me to commit this one.

I was going to do that right after this patch lands. Please assist me (hopefully last time) in landing the patch.

In D69088#1714575, @Meinersbur wrote:

In D69088#1714020, @hsaito wrote:

Personally, I like the intent. I don't foresee a clear (enough) path to get there. This leads to hesitation of adding a new non-experimental pragma and present it to programmers. If you call it experimental, it's easier for me to swallow.

Is there anything more to do than mentioning as being it experimental in the (no-patch-available-yet) docs?

In D69088#1714019, @Meinersbur wrote:

In D69088#1713933, @hsaito wrote:

Have we established general consensus for the desire to have the flexible enough loop optimization pass ordering to accomplish the outcome of the new directive, and shared vision for the path to get there? If we are making this a general clang directive, I'd like to see the vision to get there w/o depending on polly. If this is already discussed and settled, pointer to that is appreciated so that I can learn.

Response to the RFCs was meager. However, I got positive feedback at various conferences, including last year's DevMtg where my version for loop transformations was a technical talk.

Have we established general consensus for the desire to have the flexible enough loop optimization pass ordering to accomplish the outcome of the new directive, and shared vision for the path to get there? If we are making this a general clang directive, I'd like to see the vision to get there w/o depending on polly. If this is already discussed and settled, pointer to that is appreciated so that I can learn.

@Meinersbur, if I remember correctly, there was an RFC discussion on this topic, right? If yes, would you post the pointer to that? I need a refresher on what has been discussed/settled in the past.

In D67948#1710615, @ebrevnov wrote:

@hsaito, Please commit if you find this version acceptable.

In D68814#1707477, @Ayal wrote:

If conditional assumes are to be dropped, better do so on entry to VPlan, as in DeadInstructions, rather than representing them in ReplicateRecipe (as do unconditional assumes) and silencing their code generation.

To retain conditional assumes along with their control flow, they could be marked under isScalarWithPredication; but this complicates vectorization, plus what use are such assumes when all else is if-converted(?)

Conditional assumes under uniform control flow could be retained, along with the uniform control flow they depend upon; this may be mostly relevant for outerloop vectorization.

In D68651#1704375, @kparzysz wrote:

I'm in favor of treating signed saturation as canonical. The issue in delaying detection of such cases to instruction selection is the volatility of the IR: there is no guarantee that the IR will remain in the same form (expected by isel) from one day to the next. For example, some optimization may decide to just promote the operations to the wider type and only do the extension/truncate once, depending on how many saturating operations may be near one another. Handling this variability in isel is just not feasible.

LGTM. Please wait for a few days in case others have more comments.

I haven't looked at the patch in detail, but as author of at least part of the prior art cited here, I agree with the direction*. I also participated in some of the vector idioms discussions from a few years ago. There's overlap with the vector idiom problems, but as noted, these are generic (scalar too) math ops, so it's not exactly the same. We invested significantly in IR analysis and codegen for the math intrinsics, so that may have changed the thinking. I don't remember the sequence of events or if there was a dedicated llvm-dev thread for this, but the general idea is that if we have a generic intrinsic for the math and can easily invert the transform in the backend for targets/types that are not supported, try to canonicalize to the intrinsic.

In D68651#1699976, @dmgreen wrote:

We do form uadd_sat as in rL357012 and usub_sat from selects.

I really just need some way to generate sadd_sats for vectorisation. If there's a better way than this, I'm all ears :)

In D67948#1695326, @hsaito wrote:

Vectorizer code change looks fine with me. I'd like to see the comments updated, though. Any more changes needed for the LIT tests?

>> In this regard, I do not like pass managers running Analyses for a Transformation pass w/o first letting the Transformation pass inspect the incoming IR, but that's a totally different discussion and I don't have a solution for that problem.

Maybe I'm misunderstanding, but SCEV is lazy so if a transform looks at the IR and decides not to ask SCEV for trip count or call getSCEV then SCEV will not do any actual work.

Vectorizer code change looks fine with me. I'd like to see the comments updated, though. Any more changes needed for the LIT tests?

I agree with @Ayal about changing getAsAddRec for OptForSize. In general, it's better to not "Analyze" if we know we won't be using the result of analysis.

LGTM

Nice clean up of the code as well. LGTM.

In D66796#1663868, @SjoerdMeijer wrote:

That's exactly the reason why I think vectorize(disable) should disable vectorisation for that loop. I just don't see what else a user would expect.

There are two ways to think.

1. vectorize(disable) as in disable the LoopVectorize pass itself.
2. vectorize(disable) as in disabling the loop vectorization transformation

In D62997#1617197, @rengolin wrote:

Nope, Asan failures from the sanitizer commit.

Buildbot failures reported so far. Just FYI. I don't think any actions from this commit is needed.

In D62997#1614330, @hsaito wrote:

In D62997#1614288, @psamolysov wrote:

@rengolin @hsaito I understand you were busy during the version 9 pre-releasing. But if you have time and the diff looks good for you, could you land it? Thank you.

I can land this today.

In D62997#1614288, @psamolysov wrote:

@rengolin @hsaito I understand you were busy during the version 9 pre-releasing. But if you have time and the diff looks good for you, could you land it? Thank you.

Reported buildbot fails for Evgueni to follow up as needed:

LIT test has been moved to X86 subdir, in response to ps4-buildslave1 buildbot failure.

r367654 | hsaito | 2019-08-01 23:31:50 -0700 (Thu, 01 Aug 2019) | 12 lines

In D63981#1609626, @ebrevnov wrote:

Thanks for quick response @hsaito ! May I ask you to commit it as well since I don't have committer rights yet.

Indeed, this is a much cleaner fix. LGTM.

In D63981#1607984, @ebrevnov wrote:

Hi @hsaito!

Would be nice if you find time to take a look.

LGTM, pending the discussion about the exact meaning of the newly introduced "vector predicate" pragma (expect this to happen outside of this review). Please wait for another day to give others last minute opportunity to give feedback.

In D65197#1604940, @SjoerdMeijer wrote:

Friendly ping :-)

We probably need to discuss whether vectorize_predicate(enable) should (or should not) implicitly turns on vectorize(enable) or not. I guess the current behavior is "does not", right? We don't have to discuss that in this review, but we still want to make a conscious decision one way or the other, or did I miss that discussion?

LGTM

Just one comment from me.

Looks to be a good change. Can we add a little more improvement to this patch --- adding more crispness in ORE message?

Aha, that was a "hacky" way to get "loop contains a switch statement" along with the warning. I see. I suppose we can't blindly use LV_NAME, then.
I then suppose some tests (like pr38800.ll) didn't even need -pass-remarks-missed flag (which is incorrectly used, I think).

In D62997#1589461, @psamolysov wrote:

The test no_switch.ll has been updated: the LV doesn't report remarks without -pass-remarks-missed='loop-vectorize' -pass-remarks-analysis='loop-vectorize', so the test from the CHECK section is removed. I have no idea why, but remarks with lambdas through the code emitted via OptimizationRemarkMissed, not OptimizationRemarkAnalysis (if I right understand, 'Missed' means the pass has not been applied by any reasons during optimization).

Looking at the expected output and the explanations on -Rpass* flags, it could be that those tests should be using -pass-remarks-analysis=loop-vectorize, instead of -pass-remarks-missed=loop-vectorize. Would you try?

In D62997#1553998, @rengolin wrote:

In D62997#1551946, @psamolysov wrote:

The diff has been updated: private methods were removed, the function 'reportVectorizationFailure' was moved to LoopVectorize.cpp and declared in LoopVectorize.h (namespace llvm). I've removed the passName parameter and pass LV_NAME as pass name to ORE but this change breaks 3 regression tests:

I'm assuming that the pass name isn't just LV_NAME, but whichever pass is using that function, which now that it's higher level, can be anything.

@rengolin @hsaito I need you suggestions how to deal with the Hints.vectorizeAnalysisPassName() method, it is very long to always call it whenever the reportVectorizationFailure is invoked.

I don't have a better idea on the top of my head, but this sounds like a change that can be done later as a quick refactory once someone comes up with a clever replacement.

I'd be ok having that for now... @hsaito may have a better idea, though.

--renato

Sorry, I missed the original review request in the e-mail pile up. Yes, this is the direction I was suggesting. Thank you.

In D62478#1518397, @rengolin wrote:

In D62478#1518099, @psamolysov wrote:

@rengolin It is because the function uses OptimizationRemarkEmitter *ORE, a member of the LoopVectorizationLegality class.

I see. Given the function has a lot of arguments already and it really isn't used elsewhere, I'd rather just add ORE to the args list.

Unless @hsaito or @fhahn think this could be used elsewhere in the vectorizer, then it shouldn't be in that class anyway.

In D62311#1516399, @psamolysov wrote:

@hsaito I agree, to have a function to report about an error is a good idea.

I'm new in LLVM community, so what does NFC mean? Should I close this review and open a new one or you mean just to upload a new diff for comments?

While we are looking at this, I'd like to discuss how to make these things easier. I think there a merit in using a utility function that takes three strings, something along the lines of
the following pseudo code:

In D32530#1496945, @greened wrote:

What's the status of this? It seems like discussion has died down a bit. I think Graham's idea to change from <scalable 2 x float> to <vscale x 2 x float> will make the IR more readable/understandable but it's not a show-stopper for me.

Are there any other outstanding issues to address before this lands?

In D61030#1483373, @asbirlea wrote:

I verified that the two unroll flags propagate to the option set in the PassManagerBuilder. This change + the clang change in D61142 should not make any visible change for clang users.

In D61030#1479532, @asbirlea wrote:

You mean, clang setting EnableLoopInterleaving.

Actually I meant DisableUnrollLoops in the PassManagerBuilder. It's currently always set to false, not based on a flag, and I found a single instance where it's value is changed in clang (see clang patch).

I'm happy to make whatever change needed to clang first, but I don't see what that change is. If you could point me to what I've missed, that would be great!

In D61030#1479208, @asbirlea wrote:

Thanks a lot for the suggestion! Sent: http://lists.llvm.org/pipermail/llvm-dev/2019-April/131968.html

My intention was to not make any change visible to clang.
If clang currently sets the DisableUnrollLoops, and llvm will not use that for LoopVectorization, then have clang set LoopsInterleaved to the same value as the one used for unroll loops.

In D61030#1478957, @asbirlea wrote:

Update test.

In D32530#1477164, @rengolin wrote:

In D32530#1477041, @hfinkel wrote:

Exactly. Non-constant values can become constant. Constant values can be guarded by vscale-dependent runtime guards (both hand-written and compiler generated). My preference is to leave this not restricted to vscale == 1 values, but rather allow all values that can be supported at runtime, and have it be UB if, at runtime, the relevant index is not available.

+1

In D32530#1475356, @huntergr wrote:

In D32530#1474555, @greened wrote:

In D32530#1470784, @hfinkel wrote:

In D32530#1470773, @greened wrote:

I am not sure how it could be anything but n. If you don't know how long the vector is, you can't correctly generate an index beyond n.

But you know at runtime... there has to be a way to determine, at runtime, vscale. And the index doesn't need to be a constant. I'm not sure that we need to restrict non-constant n to only values valid for vscale == 1.

Good point. 100% agree. I was only considering the constant case.

Ok, so do we have agreement that constant literal indices should be limited to 0..n-1 for now, but non-constant indices can potentially exceed n so that expressions featuring vscale can be used?

In D57504#1469847, @simoll wrote:

Do we really need both vp.fadd() and vp.constrained.fadd()? Can't we just use the latter with rmInvalid/ebInvalid? That should prevent vp.constrained.fadd from losing optimizations w/o good reasons.

According to the LLVM langref, "fpexcept.ignore" seems to be the right option for exceptions whereas there is no "round.permissive" option for the rounding behavior. Abusing rmInvalid/ebInvalid seems hacky.

In D57504#1468510, @simoll wrote:

Updates

• added constrained fp intrinsics (IR level only).
• initial support for mapping llvm.experimental.constrained.* intrinsics to llvm.vp.constrained.*.

In D32530#1466746, @huntergr wrote:

In D32530#1464722, @hsaito wrote:

In D32530#1463735, @joelkevinjones wrote:

I think this is a coherent set of changes. Given the current top of trunk, this expands support from just assembly/disassembly of machine instructions to include LLVM IR, right? Such being the case, I think this patch should go in. I have some ideas on how to structure passes so SV IR supporting optimizations can be added incrementally. If anyone thinks such a proposal would help, let me know.

I think there is one more thing we still have to do. Does scalable vector type apply to all Instructions where non-scalable vector is allowed? If the answer is no, we need to identify which ones are not allowed to take scalable vector type operand/result. Some of the Instructions are not plain element-wise operation. Do we have agreed upon semantics for all those that are allowed?

The main difference is for 'shufflevector'. For a splat it's simple, since you just use a zeroinitializer mask. For anything else, though, you currently need a constant vector with immediate values; this obviously won't work if you don't know how many elements there are.

In D32530#1467094, @hfinkel wrote:

In D32530#1466762, @rengolin wrote:

In D32530#1466746, @huntergr wrote:

So if we wanted to keep them as intrinsics for now, I think we have one of three options:

1. Leave discussion on more complicated shuffles until later, and only use scalable autovectorization on loops which don't need anything more than splats.

Given the current state, this is the easiest path.

I agree, although this is an important part of the model, so we should start having this discussion in parallel (sooner rather than later). I had been under the impression that a set of intrinsics were being proposed for this, but extending shufflevector is also an option worth considering. If these are first-class types, then having first-class instruction support is probably the right path. This deserves it's own RFC.

2. Introduce additional intrinsics for the other shuffle variants as needed
3. Allow shufflevector to accept arbitrary masks so that intrinsics can be used (though possibly only if the output vector is scalable).

This warrants a larger discussion, which would hinder the current progress.

I agree. We should have a separate RFC on this.

In D32530#1463735, @joelkevinjones wrote:

I think this is a coherent set of changes. Given the current top of trunk, this expands support from just assembly/disassembly of machine instructions to include LLVM IR, right? Such being the case, I think this patch should go in. I have some ideas on how to structure passes so SV IR supporting optimizations can be added incrementally. If anyone thinks such a proposal would help, let me know.

LGTM

In D57598#1445741, @fhahn wrote:

Thanks Francesco!

In D57598#1445180, @fhahn wrote:

Thanks Francesco! I'll commit the change tomorrow, unless @hsaito does it today :)

@hsaito, I don't have commit access, could you commit this change for me?

In D57598#1440282, @fpetrogalli wrote:

@npanchen , @fhahn , gentle ping :)

Francesco

In D57978#1435473, @Meinersbur wrote:

ping

LGTM. Please wait for a few days to give others a chance to go over your updated patch.