LGTM! Thanks for this fix @sdesmalen.

LGTM! Thanks for all the good work on this so far @huntergr. :)

LGTM! Eccelente! Thanks for making the changes @CarolineConcatto.

Thanks for the new tests @CarolineConcatto! I just had a couple more suggestions on possibly improving the tests a bit more ...

• Changed patch to remove dependence on D146127

In D144274#4219580, @nikic wrote:

I've put up https://reviews.llvm.org/D146813 to move the loop invariant GEP reassociation into LICM, which should allow us to drop the LoopInfo dependency from InstCombine.

• Introduced a new TailFoldingInfo structure at the suggestion of @paulwalker-arm on D146128 as I think this makes sense.

In D144274#4219397, @tschuett wrote:

There is no space for an InstCombineWithLoopInfo to avoid all the phase ordering issues?

In D144274#4219354, @fhahn wrote:

In D144274#4216205, @david-arm wrote:

I think I've found the problem - the patch was missing another case that needs the loop info option.

great, would it be possible to add a phase ordering test that demonstrates the issue and shows that it is fixed with the latest version?

• Addressed review comments

• Addressed review comments.

• Added a missing place where we require the loop info.

I have spoken with @spatel who said he is unlikely to have much time to progress this patch and he's happy for me to commandeer it.

I think I've found the problem - the patch was missing another case that needs the loop info option.

I have spoken with @spatel who said he is unlikely to have much time to progress this patch and he's happy for me to commandeer it. I would like to make progress on this because it is an important fix on AArch64 for the SPEC2017 benchmark parest.

In D146199#4198834, @dmgreen wrote:

I had written a very similar patch recently, but it would only use the fixed length if the scalable was unknown. The performance of it was pretty bad though, so I ended up dropping it. I had noticed that there is an xfail in llvm/test/Transforms/LoopVectorize/AArch64/eliminate-tail-predication.ll at the moment. Can it now be replaced with a check for store <vscale x 4 x i32>?

TargetTransformInfo::isVScaleKnownToBeAPowerOfTwo isn't going to be useable from all the places that need it like instcombine. It might be best to add it to somewhere like vscale_range in the long run?

• Addressed review comments about using std::optional

In D146128#4198864, @dmgreen wrote:

LoopVectorizationLegality::containsDecreasingPointers seems to loop over all the instructions and call isConsecutivePtr, which just calls getPtrStride. Could that logic just be placed in AArch64TTIImpl::preferPredicateOverEpilogue? That is how it has worked in ARMTTIImpl::preferPredicateOverEpilogue via canTailPredicateLoop. Otherwise the code in containsDecreasingPointers is ran for any architecture, but only used by AArch64.

• Moved containsDecreasingPointers to AArch64TargetTransformInfo.cpp

Hi @paulwalker-arm,

• Rebased on top of NFC test patch.

In D146128#4198864, @dmgreen wrote:

LoopVectorizationLegality::containsDecreasingPointers seems to loop over all the instructions and call isConsecutivePtr, which just calls getPtrStride. Could that logic just be placed in AArch64TTIImpl::preferPredicateOverEpilogue? That is how it has worked in ARMTTIImpl::preferPredicateOverEpilogue via canTailPredicateLoop. Otherwise the code in containsDecreasingPointers is ran for any architecture, but only used by AArch64.

Adding @hassnaa-arm as a reviewer too as she has been doing a lot of work on the streaming compatible code generation.

In D144274#4186897, @spatel wrote:

In D144274#4186866, @nikic wrote:

Hm, it looks like this ended up causing non-trivial codegen changes: https://llvm-compile-time-tracker.com/compare.php?from=772aa05452f8ff90a47168e6801cda2acb5a1873&to=43ae4b62b2671cf73e691c0b53324cd39405cd51&stat=size-text So we're not preserving previous behavior in some significant way(s).

Thanks - I reverted with 43ae4b62b267.
It's unlikely that I will be able to investigate those diffs in the near term, so hopefully someone else can commandeer/edit this patch and update D144045.

Hi @sanjay, thanks for landing the patch anyway!

LGTM!

From what I can see the patch LGTM! It seems like all review comments have been addressed and this patch then unblocks D144045.

LGTM!

This patch hasn't moved for a long time. Trying to clean up my review list in Phabricator!

This patch hasn't moved for a long time. Trying to clean up my review list in Phabricator!

LGTM! Eccelente. :)

LGTM! Thanks for making the changes @dmgreen. :)

LGTM!

LGTM!

Looks sensible to me! I just had one minor comment ...

LGTM! Muy bueno!

Nice! I had a few minor comments, except for a possible issue with the use of getSmallBestKnownTC.

Hi @spatel @craig.topper, I did think about trying to change InstCombine to avoid sinking, but given resistance to this approach in the past I wasn't sure if this was the right approach. However, certainly it's unfortunate that we run InstCombine so many times in the LTO pipeline that we end up with this problem. I agree that adding another LICM pass at the end increases compilation time too, which isn't great. I think ultimately we have to decide between 1) increased compilation time without changing InstCombine, 2) reduce compilation time and increase performance by not sinking the fdiv in InstCombine. I'm not particularly attached to either approach, but I definitely do want to fix it. :)

Thanks a lot for making all the changes @bryanpkc - it's looking really good now! I just have a few minor comments/suggestions and then I think it looks good to go.

LGTM! Thanks for making all the changes @huntergr. :)

LGTM!

LGTM!

LGTM!

• Added unused parameters to the x4 tests and changed x2 tests to ensure we're testing the destination register starts at a multiple of 2.

Thanks for putting these tests in a precommit patch @sushgokh!

Hi @SjoerdMeijer, in @sushgokh's defence there is precedent for some of the changes in this patch - by changing from SVE_4_Op_Pat to SVE_4_Mad_Op_Pat we are able to set the AddedComplexity field to the pattern, which is not dissimilar to SVE_3_Op_Pat_SelZero or SVE_3_Op_Pat_Shift_Imm_SelZero, i.e.

LGTM! Thanks for making the changes. :)

Thanks a lot for addressing all the comments @huntergr! I just have a few more minor comments then I think it's good to go. :)

Hi @dmgreen, thanks for this patch - it adds some very useful functionality to the vectoriser and allows us to reduce the code size for epilogues too! I just had a few questions ...

This patch looks good and it saves us having two orthogonal states! I just had a few minor comments ...