[LV] Fix analyzeInterleaving when -pass-remarks enabled

[mtrofin]

Summary

If -pass-remarks=loop-vectorize, atomic ops will be seen by
analyzeInterleaving(), even though canVectorizeMemory() == false. This
is because we are requesting extra analysis instead of bailing out.

In such a case, we end up with a Group in both Load- and StoreGroups,
and then we'll try to access freed memory when traversing LoadGroups after having had released the Group when iterating over StoreGroups.

The fix is to include mayWriteToMemory() when validating that two
instructions are the same kind of memory operation.

[davidxl]

lib/Transforms/Vectorize/LoopVectorize.cpp

5929 ↗

(On Diff #133376)

s/exclussive/exclusive/

5933 ↗

(On Diff #133376)

Add a test case?

[hsaito]

lib/Transforms/Vectorize/LoopVectorize.cpp

5931–5932 ↗

(On Diff #133376)

The root-cause of this problem is the fact that Interleaved Access Analysis is invoked as part of legality check ---- most likely because the original developer did not want to pass one extra arg to CostModel, etc., decided to keep InterleaveInfo in Legality class, and reviewers did not object to it. I'm currently working on a change set to address this architectural problem (i.e., optimization/cost model related work done in legal) by moving interleave access analysis outside of Legality.

Having said that, tightening "same kind of memory operation" check is a good thing to have even after the above mentioned refactoring is done.

[mtrofin]

Added test.

[mtrofin]

Removed blank line.

[mtrofin]

lib/Transforms/Vectorize/LoopVectorize.cpp

5931–5932 ↗

(On Diff #133376)

Thank you for the insight!

[mtrofin]

Ping - Thanks!

[davidxl]

lgtm

llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp

for (auto AI = std::next(BI); AI != E; ++AI) {
 
       // At this point, we've checked for illegal code motion. If either A or B
       // isn't strided, there's nothing left to do.
       if (!isStrided(DesA.Stride) || !isStrided(DesB.Stride))
         continue;
 
       // Ignore A if it's already in a group or isn't the same kind of memory
       // operation as B.
-      if (isInterleaved(A) || A->mayReadFromMemory() != B->mayReadFromMemory())
+      // Note that mayReadFromMemory() isn't mutually exclusive to mayWriteToMemory
+      // in the case of atomic loads. We shouldn't see those here, canVectorizeMemory()
+      // should have returned false - except for the case we asked for optimization
+      // remarks.
+      if (isInterleaved(A) || (A->mayReadFromMemory() != B->mayReadFromMemory())
+          || (A->mayWriteToMemory() != B->mayWriteToMemory()))
         continue;
 
       // Check rules 1 and 2. Ignore A if its stride or size is different from
       // that of B.
       if (DesA.Stride != DesB.Stride || DesA.Size != DesB.Size)
         continue;
 
       // Ignore A if the memory object of A and B don't belong to the same

llvm/trunk/test/Transforms/LoopVectorize/interleaved-acess-with-remarks.ll

+; RUN: opt -S -loop-vectorize -instcombine -force-vector-width=4 -force-vector-interleave=1 -enable-interleaved-mem-accesses=true -runtime-memory-check-threshold=24 --pass-remarks=loop-vectorize < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+
+
+; This only tests that asking for remarks doesn't lead to compiler crashing
+; (or timing out). We just check for output. To be sure, we also check we didn't
+; vectorize.
+; CHECK-LABEL: @atomicLoadsBothWriteAndReadMem
+; CHECK-NOT: <{{[0-9]+}} x i8>
+
+%"struct.std::__atomic_base" = type { i32 }
+%"struct.std::atomic" = type { %"struct.std::__atomic_base" }
+%union.anon = type { i64 }
+%MyStruct = type { i32, %"struct.std::atomic", %union.anon }
+
+define void @atomicLoadsBothWriteAndReadMem(%MyStruct *%a, %MyStruct *%b, %MyStruct *%lim) {
+entry:
+  br label %loop
+
+loop:
+  %0 = phi %MyStruct* [ %a, %entry ], [ %ainc, %loop ]
+  %1 = phi %MyStruct* [ %b, %entry ], [ %binc, %loop ]
+  %2 = getelementptr %MyStruct, %MyStruct* %1, i64 0, i32 0
+  %3 = load i32, i32* %2, align 8
+  %4 = getelementptr inbounds %MyStruct, %MyStruct* %0, i64 0, i32 0
+  store i32 %3, i32* %4, align 8
+  %5 = getelementptr inbounds %MyStruct, %MyStruct* %1, i64 0, i32 1, i32 0, i32 0
+  %6 = load atomic i32, i32* %5 monotonic, align 4
+  %7 = getelementptr inbounds %MyStruct, %MyStruct* %0, i64 0, i32 1, i32 0, i32 0
+  store atomic i32 %6, i32* %7 monotonic, align 4
+  %8 = getelementptr inbounds %MyStruct, %MyStruct* %1, i64 0, i32 2, i32 0
+  %9 = getelementptr inbounds %MyStruct, %MyStruct* %0, i64 0, i32 2, i32 0
+  %10 = load i64, i64* %8, align 8
+  store i64 %10, i64* %9, align 8
+  %binc = getelementptr inbounds %MyStruct, %MyStruct* %1, i64 1
+  %ainc = getelementptr inbounds %MyStruct, %MyStruct* %0, i64 1
+  %cond = icmp eq %MyStruct* %binc, %lim
+  br i1 %cond, label %exit, label %loop
+
+exit:
+  ret void
+}