[PATCH] D108371: [LAA] Add Memory dependence and unknown bounds remarks.

[Alban Bridonneau via Phabricator via llvm-commits]

alban.bridonneau added a comment.

Thanks for the cleanup on the unit tests. It's clearer, it helps to start digging into the details of what they do.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:960-966
+      std::string LocText = " Memory location is the same as accessed at line ";
+      if (SourceLoc) {
+        LocText += std::to_string(SourceLoc.getLine()) + ":" +
+                   std::to_string(SourceLoc.getCol());
+      } else {
+        LocText += "<unknown>:<unknown>";
+      }
----------------
malharJ wrote:
> alban.bridonneau wrote:
> > We should be able to reuse existing functions to create the full location string. How about using DebugLoc::print?
> I agree, but I tried DebugLoc::print() and it ends up printing the filename (which is redundant since it is already present at the beginning of the remark), followed by line and column.
> 
> Do we want the filename to appear for each debug location we output ?
> If so, then what should the format of the output messages be ?
Yes, we want the filename to appear on both locations. The second access could have come from another file, through inlining.
It might become less readable if the path is really long, but that's something that the user will deal with anyway, if they have long file paths.


================
Comment at: llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll:27
+  %arrayidx2 = getelementptr inbounds i32, i32* %A, i64 %idxprom1, !dbg !35
+  %1 = load i32, i32* %arrayidx2, align 4, !dbg !35, !tbaa !31
+  %add = add nsw i32 %1, 1
----------------
Your tests all use the same types, I believe tbaa is not how the aliasing issues are detected, and you can remove those nodes


================
Comment at: llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll:37
+; // a) Dependence::NoDep
+; // Loop containing only reads does not hinder vectorization
+; void test_nodep(int n, int* A, int* B, int* C) {
----------------
I am not sure i understand this test. The description says the loop contains only reads, but the IR has stores in it.
Also, the IR is already vectorized, so it's not really a useful test case


================
Comment at: llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll:179
+
+vector.body:                                      ; preds = %vector.body, %vector.ph
+  %index = phi i64 [ 0, %vector.ph ], [ %index.next, %vector.body ]
----------------
same here, the IR is already vectorized. For such a patch i would have expected all loops to be scalar, as before entering the Loop Vectorizer


================
Comment at: llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll:13
+
+define dso_local void @test_unknown_bounds(i32* nocapture %A, i32* nocapture readonly %B, i32 %n) local_unnamed_addr #0 !dbg !13 {
+entry:
----------------
malharJ wrote:
> malharJ wrote:
> > alban.bridonneau wrote:
> > > Can we reduce the unit tests to only what's required?
> > > I believe the function attributes like #0 are not required. Neither are the calls to llvm.dbg.value. That should also help to reduce the massive amount of metadata at the bottom of the file.
> > Thanks for this suggestion.
> > 
> > 
> Although I've done it now,  is there some automated way/script to perform this ?
> It took me a fair amount of time to do it.
I am not aware of any tool to do this cleanup


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371