This looks good to me, thanks!

ok, please land independent changes separately, thanks!

(how) are the VPPredInstPHIRecipe changes related to the introduction of getLiveInIRValue()?

In D93317#2472174, @fhahn wrote:

LGTM, thanks! I pushed a small fix for the crash discussed earlier in 0ea3749b3cde. It should be a very safe/straight-forward fix, but I would appreciate taking a look post-review.

Good step towards representing more Values, and in particular recording InductionDescriptor information, in VPlan!

This currently requires giving access to VPValue to those classes, so we
can call the protected constructor. But most of them can be removed in
the future, once the remaining single value def recipes inherit from
VPValue.

Good catch and fix! Minor post-commit comments.

In D93317#2470271, @fhahn wrote:

I think there are some scenarios when we break some LCSSA PHIs that use PHIs created during SCEV expansion when we generate the runtime checks. After a first glance, it appears like an issue after we add the conditional branch from the middle block. The example below should cause a verifier failure with opt -loop-vectorize -force-vector-width=4. I tried to reduce it a bit, but unfortunately it is still quite ugly.

This looks fine to me, thanks; would be good to get @fhahn approval too.

Nice leverage of requiresScalarEpilogue!
Looks good to me, adding some minor comments.

This looks good to me, adding some minor comments. Thanks!

In D66720#2413499, @Kryukova_l_p wrote:

@Ayal Hi.
Found a problem caused by these changes.
Take a look, please. I posted about it here https://bugs.llvm.org/show_bug.cgi?id=47390

I attach the file here too, after the bug point:

reduced-simplified.ll18 KBDownload

Run like this:
opt -passes=loop-vectorize 'reduced-simplified.ll' -S

Could you take a look?
Thanks in advance.

This looks good to me, thanks!

In D88380#2393587, @dmgreen wrote:

OK sounds great. I did not know that MLIR could represent multiple values, that's good to see.

I still like honestly like a more "llvm" Value/User model more, but perhaps as I read the patches I will learn to like Defs too.

Looks good to me, adding minor comments, thanks for pushing this forward!

Extending the recipe abstraction to support def/use relations is an important next step forward for VPlan, thanks for pushing this momentum forward!

This looks good to me, thanks!

This looks good to me, as part of the effort to support VPlan def-use modeling and traversals.
Note that top-down traversal from VPValue to its VPUsers, will now need to check if each VPUser isa single-def recipe/VPInstruction in order to continue downwards to its VPUsers, etc., facilitating multi-def recipes.

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!