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

[Malhar via Phabricator via llvm-commits]

malharJ added inline comments.


================
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>";
+      }
----------------
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 ?


================
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:
----------------
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.




================
Comment at: llvm/test/Transforms/LoopVectorize/loopvectorize-opt-remarks.ll:23
+for.body.preheader:                               ; preds = %entry
+  %wide.trip.count = zext i32 %n to i64, !dbg !26
+  br label %for.body, !dbg !28
----------------
alban.bridonneau wrote:
> This kind of line can also be cleaned. Keep the loop control minimal, so that we can focus on the memory dependencies.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108371