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

Wed Jul 27 10:23:06 PDT 2022

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

In D130482#3678511 <https://reviews.llvm.org/D130482#3678511>, @mkazantsev wrote:

> 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, and I don't know if it's expected or not. If there is a more targeted fix, that'd be nice. I'll hold this patch for a while.

Thanks for your patience! After taking a closer look, it seems like the patch is a good first step and clearly improves the current situation. Fixing the remaining issue will require more time, and visiting the blocks in deterministic order is helpful in general.

LGTM, thanks!

================
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"
+
----------------
nit: shouldn't be needed

================
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.
----------------
nit: Remove reference to vectorizer?

================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/pr56672.ll:19
+
+bb2:                                              ; preds = %bb1
+  store i32 1, i32* %tmp7, align 4
----------------
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.

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

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

https://reviews.llvm.org/D130482