LGTM

In D99696#2856370, @mizvekov wrote:

@anhtuyen

I pushed a DR with a preliminary fix for it: https://reviews.llvm.org/D105380

This is not ready to merge, still have to add test cases and also decide between a pointed fix like this, or improving the ergonomics of getDeclAlign by returning possible failure.

But it does fix your repro.

Hi Matheus @mizvekov,
The commit 12c90e2e25dfd1d38250055efc21acb42d158912 from this patch seems to cause a regression, where an assertion failure starts to occur with testcases such as

The error only occurs when I have more code in my local. I close it now.

In D91907#2409406, @mgorny wrote:

Curious enough, I can't reproduce this failure.

Reopen due to a failed build on builder polly-x86_64-linux
Full details are available at:

https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_-23builders_10_builds_615&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=CQgGRYR_3xNbiZpMevJ_fOqMziVnPdj-Qa8sJXA1NHQ&s=2jJHm84n7dRHKQQ9ax_D-LQiQ6lf_9ypNbnuph-cCAs&e=

Buildbot URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__lab.llvm.org-3A8011_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=Nr2SnYQn80wzIpml4veK2C-U4ilVZzi0kWYCc2WSLoA&m=CQgGRYR_3xNbiZpMevJ_fOqMziVnPdj-Qa8sJXA1NHQ&s=JFUUxRP6j5j0BPDw9yCc6lMViVmJFWrmcTQN13xbtEE&e=
Worker for this Build: polly-x86_64-gce1

Address comment by Lint: Pre-merge checks related to clang-format

Hello @lebedev.ri
Bardia and I have provided the answer to your concerns, but we have not heard from you since last week. Please let me know what other changes, if any, that you still need to see.

Update with cl::location and remove the extra - from the testcase (eg from --disable... to -disable).

Bugreport: https://bugs.llvm.org/show_bug.cgi?id=47300

Thanks @bmahjour for uploading a DA test case. I had difficulty coming up with a similar test myself.
To @lebedev.ri : I think the comments from @bmahjour might have answered most of your questions. If not, please list them here and I will try to address them. If you insist in having the testcases in bugreport, I will do that. It might take some time though, since I am still waiting for my password to be reset via the webtools (contacted bugs-admin).

Hi @lebedev.ri
I saw you had again requested changes to this revision, but I could not find what changes that you thought should be required. Can you please be specific?

I removed the details about DA being confused by the insertion of memset/memcpy by LIRP in the description.

In D86262#2231209, @lebedev.ri wrote:

I'd like to see a PhaseOrdering test showing the IR that is not optimized due to the DA being unaware about these intrinsics.

Address comments by the reviewers.

To avoid any issue with NDA, I wrote a simple test as follows. This test program tries to prove the fact that, the impact of Loop Idiom Recognize Pass (LIGP)’s replacing store with memset is not always a positive one.

... a concrete motivating example would be helpful.

In D86262#2228451, @lebedev.ri wrote:

In D86262#2228420, @anhtuyen wrote:

In D86262#2227781, @dfukalov wrote:

In D86262#2227676, @lebedev.ri wrote:

Why not just enhance DA?

I agree with that, it seems to be better to improve DA. Is it feasible?

I agree that DA can be improved, but I hope that the example below from @nemanjai about libraries, which are highly tuned with respect to when/whether to call memset/memcpy functions provides another reason for this proposed change.

I'm not really sold on this. We have similar transform in InstCombine.
If the original problem is with DA, then that is what should be improved.

If this transform is a pessimization for some code, it would be good to actually see that code.

I hope you find the detailed comment below by @bmahjour (thanks, @bmahjour) a reasonable ground for this change.

In D86262#2227781, @dfukalov wrote:

In D86262#2227676, @lebedev.ri wrote:

Why not just enhance DA?

I agree with that, it seems to be better to improve DA. Is it feasible?

In D86262#2228123, @nemanjai wrote:

Presumably (now or in the future) there may be other places in this pass that produce memset/memcpy. Also, we will unnecessarily collect stores in in collectStores(). So it seems to me like it might be more advantageous to have these options prevent isLegalStore() from returning the corresponding store type.

Remove an empty line based on reviewer's comment.

Thank you very much Ayal for your suggestion. I adjusted the file accordingly.

Hello Ayal @Ayal , when you have some time, can you review this patch to the testcases, please! Thank you very much!

Extend pr45679-fold-tail-by-masking.ll with an additional RUN of -force-vector-width=1 and -force-vector-interleave=4 from https://reviews.llvm.org/D80085.

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).

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.

Hello,
I write to ask if there is any thing else in this patch you like me to address. If not, please see if you could approve it. A number of testcases I am running are affected and it will be great if I can get them pass with this patch. Thank you very much!

LGTM , too. Thanks for extending this feature.

Similar to the 2nd patch earlier, this patch includes all the changes recommended by the reviewers.
Also, I

• modified the 2nd function of the LIT test to make it clearer and simpler, and
• added ORE to confirm which pass has/does not have the impact.

In D79976#2040304, @fhahn wrote:

The issue has also been reported as https://bugs.llvm.org/show_bug.cgi?id=45943

This patch includes all the changes recommended by the reviewers - thank you very much!
I also added a new LIT test, which uses the ORE to confirm which pass has the impact.
In terms of testing, I have run a number of test-cases with it, and all were successful.

Based on discussion with Ayal, I created a quick patch to the problem of type-mismatch issue I reported above.
https://reviews.llvm.org/D79976

In D78847#2031205, @Ayal wrote:

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?

Some of this issue stems from not using the overloaded getBroadcastInstrs().

The more general issues raised are whether to apply foldTail when VF==1 in the absence of masked scalar loads/stores, and/or whether to internally turn foldTail on for small loops (due to cost considerations) when the VF and/or UF are provided externally (bypassing their cost-based selection process).

In D78847#2028930, @Ayal wrote:

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>** .

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

In D78847#2028819, @anhtuyen wrote:

In D78847#2028759, @Ayal wrote:

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

Yes, thanks for catching this!
One quick fix is indeed to set VTCMO to TCMO when State->VF == 1, instead of "splatting" it into a vector of a single element.
Thinking if fold-tail-by-masking should be restricted to work for VF>1 only, given that only vectors (loads/stores) get masked.

I also came up (and gave up) that fix last week, because it would not work for a loop whose VF is 1, but the loop bound is a vector. I will come up with an example shortly to demonstrate my thought.

In D78847#2028759, @Ayal wrote:

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

Yes, thanks for catching this!
One quick fix is indeed to set VTCMO to TCMO when State->VF == 1, instead of "splatting" it into a vector of a single element.
Thinking if fold-tail-by-masking should be restricted to work for VF>1 only, given that only vectors (loads/stores) get masked.

The new fix delivered in this patch has caused an assert failure with a testcase having a loop with a very small trip count going through fold tail by masking.
The reduced testcase is as follows. The options are: -loop-vectorize -force-vector-interleave=4