[PATCH] D108763: Use type sizes when determining dependence

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes, looks mostly fine to me. I just had a few more nits on the test.



================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/depend_diff_types.ll:3
+; RUN: opt -S -disable-output -passes='require<scalar-evolution>,require<tbaa>,require<aa>,loop(print-access-info)' < %s 2>&1 | FileCheck %s --check-prefix=CHECK-SAFE
+; RUN: opt -S -disable-output -store-to-load-forwarding-conflict-detection -passes='require<scalar-evolution>,require<tbaa>,require<aa>,loop(print-access-info)' < %s 2>&1 | FileCheck %s --check-prefix=CHECK-UNSAFE
+
----------------
nit: can you move this after the -passes='...', that way it's easier to see the difference between the two RUN lines.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/depend_diff_types.ll:17
+; CHECK-SAFE:           store i32 %7, i32* %0, align 8
+
+; CHECK-SAFE:       Forward:
----------------
Can you use CHECK-SAFE-NEXT: for all these? That way there are no other dependences that aren't caught by the CHECK lines. (same for the CHECK lines below).

nit: Can you move the CHECK lines just above `define void @backdep_type_size_equivalence(..) {`?


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/depend_diff_types.ll:35
+
+  loop:
+    %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %loop ]
----------------
nit: strange indentation.


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/depend_diff_types.ll:35
+
+  loop:
+    %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %loop ]
----------------
sdesmalen wrote:
> nit: strange indentation.
nit: odd indentation


================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/depend_diff_types.ll:100
+
+  loop:
+    %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %loop ]
----------------
nit: strange indentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108763