In D83470#2142057, @fhahn wrote:

LGTM, thanks!

In such cases, the loop vectorizer should either re-run the analysis or bail-out from vectorizing the loop, as done prior to D81345. The latter is chosen for now as the former requires refactoring.

As already discussed in D81345, ideally LV would have more flexibility to drive LAA, but this requires non-trivial refactoring. Which we should do, but until then the patch looks like a reasonable fix to the crash.

Good catch!

This looks good to me, thanks! with last couple of nits.

In D81416#2136421, @AaronLiu wrote:

In the application we try, LV refuse to vectorize due to not profitable, but if we force LV to vectorize and it will crash. Apparently there are some obstacles. There are cases that even if LV fails, SLP could succeed.

In D81416#2096034, @AaronLiu wrote:

In D81416#2095961, @spatel wrote:

IIUC, we should add a test under test/Transforms/PhaseOrdering with -O2 to show the cooperative effect of the 2 vectorizers rather than a stand-alone SLP test.
If you can push that test with full baseline CHECK lines and then apply this patch and show test diffs, that would make it much easier to tell what is intended with this patch.

Thanks for the comment. This patch does not intend to change or test phase ordering. In this patch, we interleave for small loops with scalar reductions which cannot be vectorized by LV, and later on SLP captures the opportunities. Interleaving is done by LV, and vectorization is done by SLP.

Looks good to me, thanks for following-up on this suggestion from D79976.

The proposed RecurrenceDescriptor::getReductionOpChain() method identifies horizontal reductions, a subset of the vertical reductions identified by RecurrenceDescriptor::isReductionPHI(). Being two totally independent methods, it's unclear what the latter supports that the former does not. Would it be better to have isReductionPHI() also take care of recognizing horizontal reductions, and record them as in CastsToIgnore? See also TODO commented inline.

Thanks!

In D80870#2066895, @manojgupta wrote:

D80491 has been breaking our ToT builds for a week now. Can this be submitted soon. If not please consider reverting D80491.

Alternatively we could force the computed max VF to the next-lowest power-of-2

Looks good to me. Added a minor nit.

In D79783#2044036, @Pierre-vh wrote:

• Addressed comments (see items marked as "done")
• Changed "reportVectorizationFailure" calls in "prepareTailFoldingByMasking" into simple debug prints.

Would be good to also preserve the existing "force-vector-tail-folding" behavior of "fold or do not vectorize", in addition to introducing the new "prefer-vector-tail-folding" behavior.

What is your preferred solution for this? Here are some ideas:

• Add a new command-line switch that acts like the old -prefer-predicate-over-epilog, something like -force-predicate-over-epilog
• Add an additional switch to use with -prefer-predicate-over-epilog to disable the fallback behaviour, such as -disable-scalar-epilog-fallback
• Keep the old -prefer-predicate-over-epilog behaviour and only fallback when TTI requested tail-folding

In D79976#2045766, @anhtuyen wrote:

In D79976#2045738, @anhtuyen wrote:

In D79976#2045007, @Ayal wrote:

This looks good to me, thanks! Please wait a day or so if @fhahn has further comments.

As noted, it would now be good to also extend pr45679-fold-tail-by-masking.ll with an additional RUN of -force-vector-width=1 and -force-vector-interleave=4.

It is very kind of you, Ayal @Ayal , and I thank you very much. As you have mentioned, I surely will wait a day or two to see whether Florian and Bardia might have further comments.
About pr45679-fold-tail-by-masking.ll, do you want me to add it for completeness?
With VF=1, the testcase does not go through the code changed by this patch (b/c its BackedgeTakenCount is null).

Oh, I think I know what you meant now: adding the test pr45679-fold-tail-by-masking.ll AFTER applying your patch https://reviews.llvm.org/D80085. I will do.

In D75069#2041073, @dmgreen wrote:

Ping. Anyone have any thought/comments about this one? Does it at least look like its going in the right direction, or does anyone have some better suggestions?

This looks good to me, thanks! Please wait a day or so if @fhahn has further comments.

In D80085#2040692, @fhahn wrote:

LGTM, thanks! It could be beneficial to move the UserVF handling directly to computeFeasibleMaxVF (see inline comment), but either approach should be fine!

In D80110#2044051, @fhahn wrote:

In D80110#2041354, @Ayal wrote:

Perhaps clearer to introduce getMask() as in other recipes, and check if it returns null?

Thanks Ayal, I've added & used a getMask() method in the committed version!

In D79783#2043479, @Pierre-vh wrote:

• Changing implementation of the patch following discussion
• Removed the ReportFailure argument of prepareToFoldTailByMasking. I don't think it's useful anymore, but feedback is welcome. (The only thing that annoys me is that we now print "loop not vectorized" even when we'll fallback to a scalar epilogue)

Perhaps clearer to introduce getMask() as in other recipes, and check if it returns null?

Thanks for taking care of this, only a couple of nits.

There are indeed loops which can be vectorized only with a scalar remainder loop, i.e., w/o foldTail. But the tests attached could easily be made an exception: LV handles induction variables that are live-out by pre-computing their values in the pre-header. So foldTail should work fine in their presence, provided the live-out value is computed using the original trip-count rather than the one foldTail rounds up.

In D78847#2029339, @anhtuyen wrote:

[snip]

Right, VPWidenCanonicalIVRecipe::execute() also needs to treat VF==1 differently.

I looked at that, too. It still gives us the assert at a different location. We will need a little more work to do.

The above implies setting both VStart to CanonicalIV instead of splatting, and VStep to ConstantInt::set(STy, Part) instead of ConstantVector::get(Indices), when VF==1. Would doing so pass all your tests?

In D78847#2028853, @anhtuyen wrote:

[snip]

Below is an example to demonstrate that setting VTCMO to TCMO when State->VF == 1 will not help in the case of a loop of VF 1 having a vector loop-bound.

[snip]

In this case, the operand[0] (which is %vec.iv) has type <1 x i64>. The loop-bound, however, will get the type as **i64** instead of the expected **<i64 2305843009213693951>** .

In D78847#2028708, @anhtuyen wrote:

Hello,

[snip]

In this example, the operand[0] (%induction) correctly has type i64, but the loop bound (14) is of vector type <1 x i64>

There might be multiple ways to address this assert failure. I list below a few simple ones for your reference: they might or might not be a good solution at all.

1. Option 1: Not to generate the icmp instructions for %induction. In the particular case of this testcase, these instructions seem to be redundant.
2. Option 2: If we are to generate the icmp instructions above, can we set the BackedgeTakenCount to the State depending on the type of the first operand? In cases like this one when the first operand is not a vector type, using Value *TCMO instead of Value *VTCMO might be an option.

I will open a Bugzzila and copy its link to this page when my password reset goes through.

Thanks,
Anh

This looks good to me, thanks!
Noting a minor nit.
Follow-ups could be noted in TODO's.

Thanks for breaking this up!

This analysis/utils refactoring looks good to me, thanks! Just adding some nits to clarify some presumably "add-on" parts, and possible alternatives/follow-ups.

Thanks for cleaning this up!
Just adding another follow-up.

Thanks for following-up on this!

Given that CheckingPtrGroup and PointerCheck are taken out of RuntimePointerChecking to become externally visible, would it make sense to rename them RuntimeCheckingPtrGroup and RuntimePointerCheck?

In D78847#2003543, @fhahn wrote:

LGTM, thanks!

I'll add a reduced version of the crashing case from D78847 when I recommit it.

Added test. Rebased.

Thanks for following up on this! Looks good to me with a couple of nits.

In D78210#1985815, @skatkov wrote:

I'm not sure how correct this fix is but I verified that it fixes my original problem.

Addressed comment regarding test; rebased.

Yes, that's indeed better, thanks!

This looks good to me, thanks!

Sure, the updated summary's fine :-).

Clarifying the summary a bit:

Looks good to me, thanks!

In D77635#1979648, @skatkov wrote:

Hello @Ayal, unfortunately this patch causes the functional regression.
For the test below, vectorizer decided to vectorize inner loop by 32 while it has only a couple of iteration and it causes a miscompile.
Please fix it quickly or revert the patch.

The reproducer:

; ModuleID = './repro.ll'
source_filename = "./repro.ll"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
target triple = "x86_64-unknown-linux-gnu"

@global = external global i8*

define void @hoge(i8* nonnull align 8 dereferenceable_or_null(8) %arg, i8* align 8 dereferenceable_or_null(16) %arg1) {
bb:
  %tmp = load atomic i8*, i8** @global unordered, align 8
  %tmp2 = getelementptr inbounds i8, i8* %tmp, i64 852
  br label %bb3

bb3:                                              ; preds = %bb12, %bb
  %tmp4 = phi i32 [ 1, %bb ], [ %tmp15, %bb12 ]
  %tmp5 = phi i32 [ 0, %bb ], [ %tmp8, %bb12 ]
  br label %bb7

bb6:                                              ; preds = %bb12
  ret void

bb7:                                              ; preds = %bb7, %bb3
  %tmp8 = phi i32 [ %tmp5, %bb3 ], [ %tmp10, %bb7 ]
  %tmp9 = phi i32 [ 1, %bb3 ], [ %tmp10, %bb7 ]
  %tmp10 = add nuw nsw i32 %tmp9, 1
  %tmp11 = icmp ugt i32 %tmp9, 5
  br i1 %tmp11, label %bb12, label %bb7

bb12:                                             ; preds = %bb7
  %tmp13 = mul i32 %tmp8, %tmp4
  %tmp14 = trunc i32 %tmp13 to i8
  fence release
  store atomic i8 %tmp14, i8* %tmp2 unordered, align 1
  fence seq_cst
  %tmp15 = add nuw nsw i32 %tmp4, 1
  %tmp16 = icmp ult i32 %tmp4, 240
  br i1 %tmp16, label %bb3, label %bb6
}

ran as

> opt -passes=loop-vectorize -S -o res.ll ./repro.ll

Thanks for cleaning this up! Final couple of nits.

Yes, this follows D77972 naturally :-)

Thanks for this cleanup!

Thanks!

Thanks for cleaning this up. There are additional VPlan classes in other files with such a redundant 'private'; best keep them all consistent.

Address comments, use CanonicalIV instead of overloading the original PrimaryInduction term, update some comments.

Addressed comments.

In D77635#1966625, @SjoerdMeijer wrote:

I was also drafting a patch to implement this yesterday, and it was pretty much this! So I guess that's a good sign. :-)

@SjoerdMeijer , test tail-folding-counting-down.ll introduced in D72324 now fails, as it can be vectorized with fold-tail, but is not vectorized due to cost. What's the intention of this test and how should it be changed?

The purpose of this test was to catch a regression that we were seeing when tail-predication was rejected, but then incorrectly vectorisation also wasn't happening.
In this case, I think it is good to force vectorisation with a vectorisation vector of 4 or something along those lines. I was also modifying this test, but that will do for now, and then I will pick it up later.