[PATCH] D130482: [LAA] Avoid non-determinism due to blocks order. PR56672

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev updated this revision to Diff 447571.
mkazantsev marked 5 inline comments as done.
mkazantsev added a comment.

Updated test as @fhahn has proposed and moved it to LAA tests. Floran, if you can take a look into it and confirm or disprove this is a correct fix, I'd appreciate that. I'm not familiar with LAA's algorithm, so I'm just seeing this as blackbox which behaves non-deterministically depending on blocks order which should not be the case. If there is a more targeted fix, that'd be nice. I'll hold this patch for a while.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130482/new/

https://reviews.llvm.org/D130482

Files:
  llvm/lib/Analysis/LoopAccessAnalysis.cpp
  llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll


Index: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll
===================================================================
--- /dev/null
+++ llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll
@@ -0,0 +1,39 @@
+; RUN: opt -passes='loop(loop-rotate),print-access-info' -S %s 2>&1 | FileCheck %s
+; RUN: opt -passes='loop(loop-rotate),invalidate<loops>,print-access-info' -S %s 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:1-p2:32:8:8:32-ni:2"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Make sure that vectorizer's behavior based on LAA remains the same regardless
+; of the order of blocks in loop.
+define void @test(i32* %p) {
+; CHECK-LABEL: test
+; CHECK:       Report: unsafe dependent memory operations in loop.
+; CHECK-NOT:   Memory dependences are safe
+bb:
+  br label %bb4
+
+bb1:                                              ; preds = %bb4
+  br label %bb2
+
+bb2:                                              ; preds = %bb1
+  store i32 1, i32* %tmp7, align 4
+  %tmp = add nuw i64 %tmp5, 1
+  %tmp3 = icmp ult i64 %tmp, 1000
+  br i1 %tmp3, label %bb4, label %bb11
+
+bb4:                                              ; preds = %bb2, %bb
+  %tmp5 = phi i64 [ %tmp, %bb2 ], [ 16, %bb ]
+  %tmp6 = phi i64 [ %tmp5, %bb2 ], [ 15, %bb ]
+  %tmp7 = getelementptr inbounds i32, i32* %p, i64 %tmp5
+  %tmp8 = load i32, i32* %tmp7, align 4
+  %tmp9 = add i32 %tmp8, -5
+  store i32 %tmp9, i32* %tmp7, align 4
+  br i1 false, label %bb10, label %bb1
+
+bb10:                                             ; preds = %bb4
+  unreachable
+
+bb11:                                             ; preds = %bb2
+  ret void
+}
Index: llvm/lib/Analysis/LoopAccessAnalysis.cpp
===================================================================
--- llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -27,6 +27,7 @@
 #include "llvm/Analysis/AliasSetTracker.h"
 #include "llvm/Analysis/LoopAnalysisManager.h"
 #include "llvm/Analysis/LoopInfo.h"
+#include "llvm/Analysis/LoopIterator.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ScalarEvolution.h"
@@ -2127,8 +2128,11 @@
       EnableMemAccessVersioning &&
       !TheLoop->getHeader()->getParent()->hasOptSize();
 
-  // For each block.
-  for (BasicBlock *BB : TheLoop->blocks()) {
+  // Traverse blocks in deterministic order, regardless of their storage in
+  // the loop info.
+  LoopBlocksRPO RPOT(TheLoop);
+  RPOT.perform(LI);
+  for (BasicBlock *BB : RPOT) {
     // Scan the BB and collect legal loads and stores. Also detect any
     // convergent instructions.
     for (Instruction &I : *BB) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D130482.447571.patch
Type: text/x-patch
Size: 2741 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220726/30326a83/attachment.bin>