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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll:5
+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"
+
----------------
fhahn wrote:
> nit: shouldn't be needed
Not sure, but let's try to remove.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll:7
+
+; Make sure that vectorizer's behavior based on LAA remains the same regardless
+; of the order of blocks in loop.
----------------
fhahn wrote:
> nit: Remove reference to vectorizer?
Ok


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll:19
+
+bb2:                                              ; preds = %bb1
+  store i32 1, i32* %tmp7, align 4
----------------
fhahn wrote:
> nit: test case might be slightly easier to read if the block would come after the loop header (%bb4), potentially also with more descriptive names for the blocks.
No, non-standard blocks order is what seems to be causing the bug. We can't rearrange them. Renamin will do.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll:32
+  store i32 %tmp9, i32* %tmp7, align 4
+  br i1 false, label %bb10, label %bb1
+
----------------
fhahn wrote:
> nit: could this just be an unconditional branch? If not, `bb10` could be removed.
Yup, we can.


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

https://reviews.llvm.org/D130482