In D133687#3826724, @fhahn wrote:

Did you add the extra test for the regression in the original version of the patch? It looks like it wasn't included in 2d60d7ba1a26/

Refactor `isInvariantStoreOfReduction' check

Update commit message

Fixes

Add an empty line to the test and a reference to the github issue

This is a bug fix for - https://github.com/llvm/llvm-project/issues/57572

This patch actually also enables support for the cases where we have potentially conflicting accesses that have different strides (well invariant read is technically stride equal to zero). Here's one more real-live example:

In D126772#3560561, @fhahn wrote:

@igor.kirillov do you remember why this was restriction was added originally?

Address 'nits' requests

Lots of updates related to the recent comments

@fhahn, @david-arm ping. Also I run lnt test-suite and everything is fine and 17 more loops were vectorized according to -stats

@fhahn, gentle ping :)

Fix incorrectly processed case when reduction value stored in invariant could be overwritten inside loop

Add full vector.body and middle.block checks for reduc_store and reduc_store_inside_unrolled tests

@kkwli0 Hi, is it useful to run this test outside of x86? We on aarch64 had the same problem as the on addressed by a64797b5b81963a8d4bc73eaacabfee67c85c679. Alternatively I can just // UNSUPPORTED: aarch64 instead of // REQUIRES: x86-registered-target but there is a chance those clauses will be needed for the other architectures too.

Add fix that prevents vectorization for the case when not a final reduction value stored in a loop invariant address.
Add test for this case.
Remove some unused code and rename a couple of IR variables in the reduction-with-invariant-store.ll test.
Fix incorrect phi node in reduc_store_inside_unrolled and reduc_double_invariant_store tests.

Add invariant store information to VReductionRecipe::print output

Update tests

@fhahn, yes, I have access, I'll submit changes in a moment.

Final updates

@fhahn ping

@lebedev.ri This patch addresses a different problem. Here we try to handle when a reduction value is stored in some address and if this address is invariant plus other lucky conditions are satisfied we manage to vectorize. In your example address where value is stored is not invariant at all. Nevertheless, the case is interesting.

@fhahn I created the review with tests only here - https://reviews.llvm.org/D117213. Once it is merged I'll update this one.

Update tests, move invariant store tests into separate file, add more comments

@fhahn ping

Hi, @fhahn! Since there have been several quite serious changes in the patch after your last review, I would be happy to receive you approval before merge (even though status is accepted now).

Lots of updates related to the recent comments

Update comment

Fix typo

Little extension in a new test

Add scalable reduction test

Refactoring a bit

Add test

Update tests

Update intermediate store check for ordered fadd vectorization

In D110235#3072725, @david-arm wrote:

Hi @igor.kirillov, is it also possible to get this working for ordered reductions, i.e.

float sum = 0;
for(i=0..N) {
  sum += src[i];
  dst[42] = sum;
}

when building with -O3? I think it might mean updating checkOrderedReductions to look through the store. If it looks too difficult to do as part of this patch we can always follow-up with a patch later.

Update commit message and comments
Move storeToSameAddress to ScalarEvolution
Add fadd fast test
Do not apply patch for ordered fadd reductions

Remove my old test for the change

Update test, remove redundant code

It seems like the main problem is that we potentially bail out too early at the moment when checking for reductions due to the store, but once we generate runtime checks, sinking the store may become legal (see inline comment about loads to the same address)? If that's the case, ideally we'd just sink any such loads/stores before detecting reductions once we know they can be sunk due to runtime checks, but unfortunately I do not think that's possible with the current structure/ordering.

Update comment

Updating D110235